Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebase October Rain library on Laravel 6 #456

Merged
merged 103 commits into from
Aug 9, 2020
Merged

Rebase October Rain library on Laravel 6 #456

merged 103 commits into from
Aug 9, 2020

Conversation

bennothommo
Copy link
Contributor

Please see octobercms/october#4893 for more information.

This PR represents the current state of the Rain library rebase to Laravel 6.

composer.json Outdated Show resolved Hide resolved
src/Halcyon/MemoryRepository.php Show resolved Hide resolved
src/Support/Facade.php Show resolved Hide resolved
Ben Thomson and others added 17 commits January 20, 2020 22:51
In Laravel 5.6, they changed $doubleEncode to "true" by default. As a LOT of functionality in OCMS has no $doubleEncode specified, so this is resulting in a lot of double-encoded content (ie. content for code editors or rich editors).

Our override brings the default back to "false" to allow all current functionality to work without change.
The two methods it provides are not being used anymore and simply seem to define the location of the log, which is now done in "config/logging.php" and handled by Laravel.
Since the method is protected, it would be a better fix to replace our usage and keep parity with Laravel.

Replaces 13efd83
This is needed for tests, which will use a directory structure similar to plugins and modules, but has a different namespace structure.

This change should be backwards-compatible with any other uses of the ClassLoader.
Luke Towers and others added 11 commits June 9, 2020 16:57
This extends the work done by @LukeTowers in a88b5fd by doing a search (once) for all symlinks in the base directory (or in any subdirectory if `develop.allowDeepSymlinks` is enabled)

It will create an array of sources and targets in order to translate these to URLs.

In addition, this will respect the cms.restrictBaseDir config value, in that symlinks that link to folders outside the base directory will not be recorded if restricted to the base directory, but symlinks that remain within the base directory will still work.
…ersions from accidentally pulling int he L6 update
Comment on lines +28 to +29
'password' => 'required:create|min:4|confirmed',
'password_confirmation' => 'required_with:password|min:4'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukeTowers I'm not quite sure the reasoning that @daftspunk changed this back to 2 (I presume for backwards compatibility), but if we are going to change it, I personally would rather this be min:6 or min:8 at the least. We should be following best practices for password selection, and 4 character passwords are too easy to brute-force in this day and age. (I understand we have throttling, but it can be disabled).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennothommo I checked the git history and the value was originally 4, so I don't know why @daftspunk felt the need to bump it down to 2. I would also prefer the minimum be set to 8, but I believe we had a conversation about this a while ago and it was decided that people could always just bump up the rules themselves. Additionally, this is somewhat tied into the autogeneration of admin passwords PR, was that one ever finished?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukeTowers I do recall that conversation. On further consideration though, I feel if anything we should be bumping it high and forcing people to work to make the value lower, so that they are fully responsible for the consequences.

Re: auto-generation of admin passwords, it's more or less finished, but on hold until after L6.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have another convo with @daftspunk about it and get back to you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke to Sam and he re-iterated that

it's best to leave it as 4 otherwise it's a breaking change. Password policies are a complex topic and so it's best not to have any strong opinions and leave it up to a plugin or attend to the entire problem holistically
there are legit applications that use short passwords
Namely demo apps and temporary passwords
A config item would probably appease whatever is driving this. Default without a breakage

So we could add a new config item in auth.minPasswordLength that defaults to 8 in the file but in the Config::get() it defaults to the existing 4. That way existing installs won't break, and new ones will default to 8.

Also we might want to look at namespacing the auth config under auth.backend instead to better support any future changes bringing us closer to Laravel's auth system.

Luke Towers and others added 4 commits July 20, 2020 11:59
Adds a resolve_path() method that operates very similar to realpath() except in that it will allow missing files and folders. It will resolve relative directory markers and expand any symlinks encountered.

Refs: octobercms/october#5229
@LukeTowers LukeTowers modified the milestones: v1.0.468, v1.0.469 Aug 9, 2020
Ben Thomson and others added 2 commits August 9, 2020 03:23
)

Adds the config value 'app.loadDiscoveredPackages' (default: false) - if true, packages discovered through Laravel's autodiscovery feature will be loaded. The reason for this control is that in some cases, packages which are autodiscovered belong to plugins. When the plugins themselves are disabled, the discovered package is still being loaded. See rainlab/debugbar-plugin#34 for an example.

Additionally, in certain cases (for example, with Composer), sites will break when a package removed by Composer is loaded. This is because discovered packages and services are stored in cache ("storage/framework/services.php" and "storage/framework/packages.php").

This change will attempt to use the normal mechanism to load packages and services, and if it fails for any reason, it will delete the package and service cache and attempt to re-load the packages and services one more time (emulating someone running "php artisan config:clear" before "php artisan package:discover"). If a failure still occurs, then the failure is made known to the user.

Refs: #492 (comment)
@LukeTowers LukeTowers merged commit cd143d9 into develop Aug 9, 2020
@bennothommo bennothommo deleted the wip/laravel-6 branch August 10, 2020 09:33
@bennothommo bennothommo restored the wip/laravel-6 branch August 10, 2020 09:34
@bennothommo bennothommo deleted the wip/laravel-6 branch August 10, 2020 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants