Fix environment loader

[pquerner]

Description (*)

This PR attempts to fix the incomplete attempt for config values to be overwritten from ENV variables.

Related Pull Requests

Fixed Issues (if relevant)

• fixes Issues with ENV-variables override #4257 (comment)

Manual testing scenarios (*)

1. Variables from the ENV should now be disabled for editing on the backend, but still shown.
2. Variables from the ENV should be available from Mage::getStoreConfig.

Automated tests:

1. ddev phpunit --group Mage_Core_EnvLoader

Questions or comments

Please give feedback to this WIP solution.
Maybe in the future we can add a reasoning of "why" the field in the backend is disabled for edits. Such as give a "warning" sign and show that this field is overwritten by a environment variable.
I feel that this is out of scope for now, for I think that the feature should first work the best it can.

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)

[pquerner]

@sreichel Please see this PR.
(From your comment on #4257 (comment))

[sreichel]

Quick test. 404 one every page.

[sreichel]

Maybe it come from invalid setting .... ?

web_environment:
    - MAGE_IS_DEVELOPER_MODE=1
    - OPENMAGE_CONFIG__DEFAULT__GENERAL__STORE_INFORMATION__NAME=ENV default
    - OPENMAGE_CONFIG__DEFAULT__GENERAL__FOO-BAR__NAME=ENV default dashes
    - OPENMAGE_CONFIG__DEFAULT__GENERAL__FOO_BAR__NAME=ENV default underscore
    - OPENMAGE_CONFIG__DEFAULT__GENERAL__ST=ENV default invalid
    - OPENMAGE_CONFIG__WEBSITES__BASE__GENERAL__STORE_INFORMATION__NAME=ENV website
    - OPENMAGE_CONFIG__WEBSITES__BASE-AT__GENERAL__STORE_INFORMATION__NAME=ENV website dashes
    - OPENMAGE_CONFIG__WEBSITES__BASE_CH__GENERAL__STORE_INFORMATION__NAME=ENV website underscore
    - OPENMAGE_CONFIG__WEBSITES__BASE__GENERAL__ST=ENV website invalid
    - OPENMAGE_CONFIG__STORES__GERMAN__GENERAL__STORE_INFORMATION__NAME=ENV store
    - OPENMAGE_CONFIG__STORES__GERMAN-AT__GENERAL__STORE_INFORMATION__NAME=ENV store dashes
    - OPENMAGE_CONFIG__STORES__GERMAN_CH__GENERAL__STORE_INFORMATION__NAME=ENV store underscore
    - OPENMAGE_CONFIG__STORES__GERMAN__GENERAL__ST=ENV store invalid

[pquerner]

For reference my old config:

#web_environment: [
#    "OPENMAGE_CONFIG__DEFAULT__GENERAL__STORE_INFORMATION__NAME=asd_env",
#    "OPENMAGE_CONFIG__WEBSITES__BASE__GENERAL__STORE_INFORMATION__NAME=website_env",
#    "OPENMAGE_CONFIG__STORES__GERMAN__GENERAL__STORE_INFORMATION__NAME=store_german_env"
#]

With yours I am getting infinite loop.
infinteloop.html.zip

[pquerner]

The error was caused by the Mage::logException call - which finds the file via getConfigStore - which creates an infinite loop.
I've added Mage::registry checks, so that these methods dont run more than needed and removed the logException calls to counter this error.

via 1f6b7ed

[sreichel]

Thanks for working on it.

Cant make a PR to your repo right now ... how about to filter env vars in getEnv()?

    public function getEnv(): array
    {
        if (is_null($this->envStore)) {
            $env = getenv();
            $env = array_filter($env, function ($key) {
                return str_starts_with($key, self::ENV_STARTS_WITH);
            }, ARRAY_FILTER_USE_KEY);

            $this->envStore = $env;
        }
        return $this->envStore;
    }

Maybe add an additional check for env OPENMAGE_CONFIG_OVERRIDE_ALLOWED to enable/disable that feature?

[sreichel]

1. Variables from the ENV should now be disabled for editing on the backend, but still shown.

Yes, on default scope, but not on one website/storeview level.

(Scope label should ben changed to "ENV")

[sreichel]

Note ... Tests should be updated.

[pquerner]

1. Variables from the ENV should now be disabled for editing on the backend, but still shown.

Yes, on default scope, but not on one website/storeview level.

(Scope label should ben changed to "ENV")

What do you mean by that?

You want "Store Name" to be "ENV Store Name"?

All your other suggestions have been added.

Thanks. :)

//edit
To enable this, you would need this now:

web_environment:
// ...
    - OPENMAGE_CONFIG_OVERRIDE_ALLOWED=1

[sreichel]

What do you mean by that?

In "Default" scope the attribute is locked. For "German" storeview its not.

You want "Store Name" to be "ENV Store Name"?

No, not that other "label". :) "[STORE VIEW]" could change to "[ENV]" and the checkbox can be removed.

I am busy for the next 2-3 days. I'll test ASAP.

[pquerner]

In "Default" scope the attribute is locked. For "German" storeview its not.

Hm. For me its locked.

No, not that other "label". :) "[STORE VIEW]" could change to "[ENV]" and the checkbox can be removed.

Gotcha. Good point.

[sreichel]

Hm. For me its locked.

Not sure about it, sorry. Maybe the disabled checkbox was missleading. (?)

[pquerner]

Its now this:

[sreichel]

Great! ❤️

I can make a PR later, but if you have time for it, please check the red SonarCloud issues.

[pquerner]

Great! ❤️

I can do it later, but if you have time for it, please check the red SonarCloud issues.

I've removed one case.
The last ones are: unused variable (from the list) and the use of Reflection class.

I'm not sure how else to get there (without makeing the cache array public?)

[sreichel]

To pass the complexity-check it should be enough to move try/catch-parts to a seperate method.

Leave it as it is, i'll add a PR later.

PS: please composer run php-cs-fixer:fix .... :P

[pquerner]

ddev php-cs-fixer
Loaded config default from "/var/www/html/.php-cs-fixer.dist.php".

In RuleSets.php line 65:
                                    
  Set "@PER-CS2.0" does not exist.

:/

[sreichel]

@pquerner can you please take a look at the failing unit tests?

• tests for website base-at can be removed as dashes are not allowed in website code
• i think test fail b/c validition checks $xmlStruct = $this->getTestXml(); instead of system values
  • when adding website base_ch and storeviews german-at and german_ch (dashes are allowed here ?!) tests pass, so everything works fine 👍
  • guess you have to add getTestXml() to tests setup() and system config

[sreichel]

Thinks its ready to review?

[Copilot]

Pull Request Overview

This PR implements environment variable-based configuration overrides for OpenMage, allowing system configuration values to be set via environment variables with the format OPENMAGE_CONFIG__<SCOPE>__<SECTION>__<GROUP>__<FIELD>. This feature must be explicitly enabled by setting OPENMAGE_CONFIG_OVERRIDE_ALLOWED=1.

Key changes:

• Added a feature flag (OPENMAGE_CONFIG_OVERRIDE_ALLOWED) to control environment variable override functionality
• Extended override logic to support default, website, and store scopes with proper inheritance
• Added registry-based caching to prevent multiple overrides during a single request
• Created helper methods hasPath() and getAsArray() for configuration access
• Updated admin UI to display [ENV] label and disable fields overridden by environment variables

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
app/code/core/Mage/Core/Helper/EnvironmentConfigLoader.php	Core implementation of environment variable override logic with scope handling and caching
app/code/core/Mage/Core/Helper/EnvironmentConfigLoader/Override.php	New data object class for storing parsed environment variable configuration
app/code/core/Mage/Adminhtml/Block/System/Config/Form.php	Admin UI integration to display ENV scope label and disable fields
app/code/core/Mage/Adminhtml/Model/Config/Data.php	Merges environment config into configuration data retrieval
app/code/core/Mage/Core/Model/Store.php	Triggers environment override when fetching store configuration
app/code/core/Mage/Core/Model/App.php	Clears registry cache when reinitializing stores
tests/unit/Mage/Core/Helper/EnvironmentConfigLoaderTest.php	Comprehensive unit tests for environment override functionality
cypress/e2e/openmage/backend/system/config/general/general.cy.js	End-to-end test verifying ENV overrides in admin UI
cypress/support/openmage/backend/system/config/general/general.js	Test configuration for general system settings
cypress/support/openmage/_utils/validation.js	Conditional CSS validation check
cypress/support/openmage/_utils/test.js	Registers general config test namespace
cypress/support/openmage/_utils/admin.js	Adds helper function for switching config scope
cypress/support/e2e.js	Imports general config test support
cypress/e2e/openmage/frontend/customer/account.cy.js	Updates test expectation to match ENV-overridden value
.github/workflows/cypress.yml	Configures ENV variables for CI testing

[pquerner]

Website dash (-) can be removed, yes. Tests fail because they iterate through the stores and no stores with german-at or german_ch exists (anymore?).
Idk need to look into it.

[sonarqubecloud]

Quality Gate failed

Failed conditions
5.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[sreichel]

Merged.

This feature needs to be enabled via ENV OPENMAGE_CONFIG_OVERRIDE_ALLOWED.