-
-
Notifications
You must be signed in to change notification settings - Fork 435
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 feature: override configurations with env variables #3863
New feature: override configurations with env variables #3863
Conversation
…b.com/pquerner/magento-lts into feature/ENV-variables-override-config
The constructor of Mage_Core_Model_Config inherits from Varien_Object and its first argument is written to $this->data. So when the "constructor" of Mage_Core_Model_Config kicks in, it wanted to write an array to a already populated $this->_data variable, which yields errors since PHP 7.1.
I'm testing this and it looks fine. I'm wondering, if I've a env variable like
then I get Warning: Undefined array key 3 in /Users/fab/Projects/openmage/app/code/core/Mage/Core/Helper/EnvironmentConfigLoader.php on line 76
because there's no check on the amount of underscores the variable has to have in order to be considered valid |
Should be fixed via 36e55c7, however I thought that was a simple user error and should not be caught. |
i would have just counter the underscores, if it's less than X then discard the variable, isnt' regex a bit too power hungry for this? |
I tried that, it must be 4 in default or 5 in store and website. But then again, you don't know if some field name contains double underscores for whatever reason. Or storename etc. The list is long. Hence why a proper spec is needed. Or, this could be v1 and when someone has a different setup it needs to be updated. But as far as v1 I think thats good enough. Regexing a short amount of string should not be a more timely issue than counting double underscores imho. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested, works as expected and the performance hit seems negligible
Maybe we should allow |
not super common but you're right |
now includes "-" and "_" as default allowed values next to A-Z (regexp)
this only tests group, but the pattern is the same
i was thinking, do all of the methods in Mage_Core_Helper_EnvironmentConfigLoader have to be public? |
Probably not. I'll check and change visibility accordingly. I've probably done it for the tests? |
…ingly and add methods parameter type to declaration
@fballiano Changed some methods to visibility protected and fixed tests accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful
Description (*)
This PR attempts to implement Magento 2's feature of overwriting config values by the $_ENV variable.
The Magento 2 feature is documented here [1].
Related Pull Requests
Fixed Issues (if relevant)
getConfig()
reference env variables that could override values specified in XML or the database? #643Manual testing scenarios (*)
All steps assume you are using ddev as development enviroment.
web_enviroment
setting:web_environment: ["OPENMAGE_CONFIG__DEFAULT__GENERAL__STORE_INFORMATION__NAME=default", "OPENMAGE_CONFIG__WEBSITES__BASE__GENERAL__STORE_INFORMATION__NAME=website", "OPENMAGE_CONFIG__STORES__GERMAN__GENERAL__STORE_INFORMATION__NAME=store_german"]
web_environment
(Else you can also set the variables in your server configuration, either in nginx or apache2 config)
Automated testing scenarios (*)
I've given a PhpUnit test file to execute. The tests check if all scopes are correctly overwritten after calling the new EnvironmentLoader helper.
Questions or comments
This feature was requested in #643 and @Flyingmana asked me to take a look at it.
Its use-case must be documented. @Flyingmana might be able to help out with that.
ToDo / To be discussed further
I think the feature must be better implemented and documented (perhaps) than the original M2 feature-set.
For example its a little unclear (I didnt take a peek at M2 implementation) what should happen if you wish to "unset"/"delete" (?) a config value.
For now I want to gather some needed feedback.
Questions for example I have:
[1] https://experienceleague.adobe.com/docs/commerce-operations/configuration-guide/paths/override-config-settings.html
Contribution checklist (*)
(did none here, if someone can fix it pre-merge that would be great. Last time I executed that
yarn all-contributors
I didnt get added properly)This was previously open at #3842 but I've rebased something incorrectly and could not fix it another way.