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