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

Feature/env variables override config #3842

Closed

Conversation

pquerner
Copy link
Contributor

@pquerner pquerner commented Feb 18, 2024

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)

@github-actions github-actions bot added the Component: Core Relates to Mage_Core label Feb 18, 2024
@fballiano
Copy link
Contributor

@pquerner
Copy link
Contributor Author

I dont have these fails locally on 8.1.27. Not sure whats broken.

@pquerner
Copy link
Contributor Author

Anyone know what the problem is on the tests on PHP8?

@fballiano fballiano marked this pull request as draft February 29, 2024 09:34
@github-actions github-actions bot added Component: Catalog Relates to Mage_Catalog Template : admin Relates to admin template Component: lib/Varien Relates to lib/Varien Component: Sales Relates to Mage_Sales Component: Adminhtml Relates to Mage_Adminhtml Mage.php Relates to app/Mage.php Component: ImportExport Relates to Mage_ImportExport Component: Dataflow Relates to Mage_Dataflow Component: lib/* Relates to lib/* composer Relates to composer.json JavaScript Relates to js/* phpstan labels Mar 2, 2024
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.
@pquerner pquerner marked this pull request as ready for review March 2, 2024 13:21
@pquerner
Copy link
Contributor Author

pquerner commented Mar 2, 2024

Unittests have been fixed.

@fballiano
Copy link
Contributor

It seems something went wrong with the rebase because the PR has 26 modified files :-\

@pquerner
Copy link
Contributor Author

pquerner commented Mar 2, 2024

Oh darn, you're right. Let me see if I can fix that.

@pquerner
Copy link
Contributor Author

pquerner commented Mar 2, 2024

No idea, and I don't have a backup branch sadly. Maybe close and open again from the branch? The remote branch on my fork doesnt have that many changed files. Or do you see another way @fballiano ?

@pquerner pquerner closed this Mar 2, 2024
@pquerner pquerner deleted the feature/ENV-variables-override-config branch March 2, 2024 13:52
@pquerner pquerner restored the feature/ENV-variables-override-config branch March 2, 2024 13:53
@pquerner
Copy link
Contributor Author

pquerner commented Mar 2, 2024

Re-opened the PR here #3863

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: Core Relates to Mage_Core Component: Dataflow Relates to Mage_Dataflow Component: ImportExport Relates to Mage_ImportExport Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Sales Relates to Mage_Sales composer Relates to composer.json JavaScript Relates to js/* Mage.php Relates to app/Mage.php phpstan Template : admin Relates to admin template
Projects
None yet
8 participants