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

Config Overrides #10573

Merged
merged 18 commits into from
Apr 6, 2022
Merged

Config Overrides #10573

merged 18 commits into from
Apr 6, 2022

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Feb 15, 2022

What it does:

  • Implements an abstract craft\base\Config for DbConfig, GeneralConfig to extend.
  • On init, it checks all of it's public properties and:
    • Checks for an env var override, if ENV_PREFIX is set (snake_cased and uppercase).
    • If the value is a string, and the prop is typed as an array, it will split it for you (eg CRAFT_DISABLED_PLUGINS=foo,bar)
      • As we were doing this manually for a few props, I was able to remove some redundant code

Notes:

  • An alternative approach might be to make all props protected and implement a __get. I did it this way so it it only ever happens once, on init (or if you explicilty call normalize).
  • I set the ENV_PREFIX for GeneralConfig to CRAFT_. Since that is shared with a few constants (and App::env now checks constants) we just need to make sure we don't have a constant and a general config prop named the same thing…or we could set the prefix to CRAFT_GENERAL_, but that seemed a bit lame.
  • I set the DbConfig prefix to DB_, since that is what our starter uses, but it could just as easily be CRAFT_DB_.
  • Now that we have a base config class, it might make sense to move the renamedSettings logic there.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@timkelty timkelty marked this pull request as ready for review February 15, 2022 21:05
@timkelty timkelty changed the title Config overrides PoC Config Overrides Feb 16, 2022
@timkelty
Copy link
Contributor Author

timkelty commented Feb 16, 2022

Added tests 🌈

@timkelty
Copy link
Contributor Author

timkelty commented Feb 16, 2022

Was curious how expensive the reflection was, so I did a quick xdebug profile:

4.0 branch
no-overrides

feature/config-overrides branch:
overrides

So 2.4ns vs 44ns, which out of context might seem like a lot – but we're talking nanoseconds, and it only ever gets called once.

# Conflicts:
#	src/config/GeneralConfig.php
@timkelty timkelty requested a review from a team as a code owner March 15, 2022 17:47
@brandonkelly
Copy link
Member

  • If the value is a string, and the prop is typed as an array, it will split it for you (eg CRAFT_DISABLED_PLUGINS=foo,bar)

How would this handle properties that can be either a string or an array, such as loginPath, where there may be different values for each site?

Also, seems like plugin settings should be factored in, somehow.

@timkelty
Copy link
Contributor Author

timkelty commented Apr 5, 2022

How would this handle properties that can be either a string or an array, such as loginPath, where there may be different values for each site?

Because those are just typed as mixed and don't have array in their declared type, they would always just get parsed as string or bool scalars. (added a test to illustrate)

I was considering anything beyond a simple csv string to array out of the scope of this, as that comes with more baggage, and there are other tools that already do it: https://github.com/josegonzalez/php-dotenv, https://packagist.org/packages/symfony/dotenv.

If someone needed to set a complex var like via env like this, I would just suggest they do it themselves in general:

[
  'loginPath' => ['siteA' => App::env('SITE_A_LOGIN_PATH'), 'siteB' => App::env('SITE_B_LOGIN_PATH')]
]

We could go down the road of parsing these, or implement env dot notation, but that seems like a whole other concern.

@timkelty
Copy link
Contributor Author

timkelty commented Apr 5, 2022

Also, seems like plugin settings should be factored in, somehow.

This doesn't happen automatically right now, but any plugin could opt-in by making a Config class that extended craft\base\Config, and setting BaseConfig::ENV_PREFIX.

@timkelty
Copy link
Contributor Author

timkelty commented Apr 5, 2022

Re: plugins – we could check for a Plugin::configClass prop and load it from them if they have it set: https://github.com/craftcms/cms/blob/develop/src/services/Plugins.php#L932-L935

@brandonkelly brandonkelly merged commit 38e06eb into 4.0 Apr 6, 2022
@brandonkelly brandonkelly deleted the feature/config-overrides branch April 6, 2022 06:20
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.

None yet

2 participants