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

New Config class #2271

Merged
merged 3 commits into from
Sep 25, 2020
Merged

New Config class #2271

merged 3 commits into from
Sep 25, 2020

Conversation

franzliedke
Copy link
Contributor

This extracts another real class for dealing with the configuration options stored in config.php.

Similar to #2142, where I introduced a Paths class. The idea is to reduce the scope of the Application class and make it easier to inject exactly what's needed (rather than an array, which is complicated, or the bloated Application class).

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Overall I like these changes a lot, much nicer now that we can inject the config. Left a few small comments here and there

src/Foundation/Config.php Outdated Show resolved Hide resolved
src/Foundation/Config.php Outdated Show resolved Hide resolved
src/Foundation/Application.php Show resolved Hide resolved
src/Foundation/InstalledApp.php Show resolved Hide resolved
src/Foundation/UninstalledSite.php Show resolved Hide resolved
src/Console/ConsoleServiceProvider.php Show resolved Hide resolved
@askvortsov1 askvortsov1 merged commit 50cbb7b into master Sep 25, 2020
@askvortsov1 askvortsov1 deleted the fl/laravel-updates-config branch September 25, 2020 15:22
@luceos
Copy link
Member

luceos commented Sep 28, 2020

An unforeseen consequence of making the url required in the Config class is that the console install command no longer works. In fact nightly also is broken due to this change. If you can propose/recommend a solution I'm willing to patch core @franzliedke. Personally I am not sure, because making url optional in the Config reduces the need for that class altogether.

Maybe we should have different classes per environment, but that feels unnecessary as well.

What do you think?

@askvortsov1
Copy link
Sponsor Member

Perhaps we should just make it not required again? Or pass in an array of "required keys" to the constructor (with the default being ['url'])?

Either way, this is another reason why we need more robust e2e testing, so it's good to see us moving towards that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants