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

DRAFT: Fix/environment loader #4617

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pquerner
Copy link
Contributor

@pquerner pquerner commented Feb 8, 2025

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)

Relates to issue #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.

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)

@github-actions github-actions bot added Component: Core Relates to Mage_Core Component: Adminhtml Relates to Mage_Adminhtml labels Feb 8, 2025
@pquerner
Copy link
Contributor Author

pquerner commented Feb 8, 2025

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

@sreichel
Copy link
Contributor

sreichel commented Feb 8, 2025

Quick test. 404 one every page.

@sreichel
Copy link
Contributor

sreichel commented Feb 8, 2025

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
Copy link
Contributor Author

pquerner commented Feb 9, 2025

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
Copy link
Contributor Author

pquerner commented Feb 9, 2025

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
Copy link
Contributor

sreichel commented Feb 10, 2025

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 vor env OPENMAG_CONFIG_OVERRIDE_ALLOWED to enable/disable that feature?

@sreichel
Copy link
Contributor

sreichel commented Feb 10, 2025

  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
Copy link
Contributor

Note ... Tests should be updated.

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: Core Relates to Mage_Core new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants