New feature: override configurations with env variables

[pquerner]

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)

1. Fixes Is the community open to a backward compatible PR that would have getConfig() reference env variables that could override values specified in XML or the database? #643

Manual testing scenarios (*)

All steps assume you are using ddev as development enviroment.

1. In .ddev/config.yml add or fix 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"]
2. Go to adminhtml and check system configuration under "Configuration" > "General" > "Store-Information"+
3. Values from the database or default (config.xml) scope should be overwritten with given values from 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. Is the implementation okay? (I somehow feel like Helper is the wrong class construct to use here, but I don't see a "Model" here either)
2. See above (override config should delete/unset?)

[1] https://experienceleague.adobe.com/docs/commerce-operations/configuration-guide/paths/override-config-settings.html

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

(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.

[fballiano]

I'm testing this and it looks fine.

I'm wondering, if I've a env variable like

$env["OPENMAGE_CONFIG__DEFAULT__GENE"] = 'ciao ciao';

then I get

Warning: Undefined array key 3 in /Users/fab/Projects/openmage/app/code/core/Mage/Core/Helper/EnvironmentConfigLoader.php on line 76

#0 /Users/fab/Projects/openmage/app/code/core/Mage/Core/Helper/EnvironmentConfigLoader.php(76): mageCoreErrorHandler(2, 'Undefined array...', '/Users/fab/Proj...', 76)
#1 /Users/fab/Projects/openmage/app/code/core/Mage/Core/Model/Config.php(437): Mage_Core_Helper_EnvironmentConfigLoader->overrideEnvironment(Object(Mage_Core_Model_Config))
#2 /Users/fab/Projects/openmage/app/code/core/Mage/Core/Model/App.php(445): Mage_Core_Model_Config->loadEnv()
#3 /Users/fab/Projects/openmage/app/code/core/Mage/Core/Model/App.php(347): Mage_Core_Model_App->_initModules()
#4 /Users/fab/Projects/openmage/app/Mage.php(760): Mage_Core_Model_App->run(Array)
#5 /Users/fab/Projects/openmage/index.php(56): Mage::run('', 'store')
#6 {main}

because there's no check on the amount of underscores the variable has to have in order to be considered valid

[pquerner]

Should be fixed via 36e55c7, however I thought that was a simple user error and should not be caught.

[fballiano]

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?

[pquerner]

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.
Since the first guard (starts_with) is still in place.

[fballiano]

tested, works as expected and the performance hit seems negligible

[pquerner]

Maybe we should allow -? Its not quite uncommon to have - in field,group,section or store_code, yes?

[fballiano]

not super common but you're right

[fballiano]

i was thinking, do all of the methods in Mage_Core_Helper_EnvironmentConfigLoader have to be public?

[pquerner]

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?

[pquerner]

@fballiano Changed some methods to visibility protected and fixed tests accordingly.

[fballiano]

beautiful

[boesbo]

This feature is very useful! Will there be a release soon? @sreichel

[ADDISON74]

@boesbo - This PR merged on April 30 should be found in OM v20.10.2, which was released on July 27.

[boesbo]

From my tests, this PR does not work. The function loadEnv() and thus also the function overrideEnvironment are never executed. This is because $this->loadModulesCache() always returns true.
Do you know why?

[pquerner]

Did you clear cache?

[boesbo]

You are right. My comment above is invalid. Because the cache is rebuilt immediately after cleaning it, and not at the first request as I thought.

I'm testing anyway, because these environment variables, although present and loaded by the overrideEnvironment function, don't seem to be used. I'm trying to see if it's doing something wrong:

OPENMAGE_CONFIG__DEFAULT__WEB__SECURE__BASE_URL "https://test.com";
OPENMAGE_CONFIG__DEFAULT__WEB__UNSECURE__BASE_URL "https://test.com";
OPENMAGE_CONFIG__DEFAULT__WEB__COOKIE__COOKIE_DOMAIN "test.com";

[pquerner]

That looks correct.

Do you have values in the database for these fields?

I noticed for me the overrides only work when nothing is set in the database for them.. Wierd, I thought I had this tested.
Looks like this needs a fix.

// Edit

I looked a bit into it, but don't yet see a way how to solve this. If anyone wants to take a look, be my guest.
So far I saw that \Mage_Core_Model_Store::$_configCache still has the old value and needs reset. But I can't get to the store (via Mage::app()->getStore()), since it might be undefined.

Registry cannot be used, thats too late and is void for the next request.
Best is to save it to the cache I reckon.

[boesbo]

Yes, I did various tests, both with an empty db and with an empty db and local.xml, for that specific configuration. I am also doing tests because I am very interested in this, and as soon as I find the reason I will share it.
Thanks!

[sreichel]

Also did some tests ...

• not working if DB value exists
• Mage::getStoreConfig() does not work
• store config with overriden values should not have set "use default"
• store config with overriden values should be locked (to make it impossible to override ENV vars with config)
• test fail for live data (rewritten tests to use installed OM)

[colinmollenhour]

store config with overriden values should be locked (to make it impossible to override config with ENV var)

Are you saying core_config_data should take precedence over ENV at non-default scopes? In this simple implementation I thought it was good that ENV could not be overwritten, but I can imagine cases where this is both desired and not desired. Some ideas:

• If an ENV value exists, disable the form fields in the GUI to indicate that they cannot be overridden.
• Allow the ENV definer to decide: use a variant of the ENV variable name so they can be defined as overridable or not. E.g. "OPENMAGE_CONFIG*" can be overwritten but "OPENMAGE_FINAL_CONFIG*" cannot.

[sreichel]

@colinmollenhour no. My bad. Updated my comment. The other way ... like this

If an ENV value exists, disable the form fields in the GUI to indicate that they cannot be overridden.