Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Process PHP configs using the ContainerConfigurator#54970
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedMay 17, 2024
Thank you@MatTheCat. |
MatTheCat commentedMay 17, 2024
…that was fast! @yceruto does this PR seem okay to you? |
| { | ||
| foreach ($configBuildersas$configBuilder) { | ||
| $this->loadExtensionConfig($configBuilder->getExtensionAlias(),$configBuilder->toArray()); | ||
| $containerConfigurator->extension($configBuilder->getExtensionAlias(),$configBuilder->toArray(),$this->prepend); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I still need to do some config prepending tests, but I feel that we've missed something here as the previous$this->loadExtensionConfig() is currently doing more things when prepend argument istrue, e.g. the nested importing might be broken...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I saw you made theContainerConfigurator able to prepend in#52636 so I assumed it was okay but it is indeed missing this part:
symfony/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
Lines 238 to 245 in7c956d8
| if ($this->importing) { | |
| if (!isset($this->extensionConfigs[$namespace])) { | |
| $this->extensionConfigs[$namespace] = []; | |
| } | |
| array_unshift($this->extensionConfigs[$namespace],$config); | |
| return; | |
| } |
Don’t know if it is an issue at all though 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, it'll be an issue when want to prepend nested configs using imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After some quick tests, the root issue should be mitigated by processing only the config value this way:
$this->loadExtensionConfig($configBuilder->getExtensionAlias(), ContainerConfigurator::processValue($configBuilder->toArray()));
that will keep the prepending strategy working again. Would you mind creating another PR to update it? I can do it otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you already have the tests I think you’re more reading than I am; go ahead 😁
WouldContainerConfiguration::extension serve a purpose then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would ContainerConfiguration::extension serve a purpose then?
It should be avoided internally where recursive importing is involved, as now importing fromprependExtension() is supported (which requires an specific holding strategy to keep the importing order). However, you can still use it on your end for trivial array config importing (either in theprependExtension() orloadExtension() methods).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okaaay thanks; glad I asked your review 😄
yceruto commentedMay 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@MatTheCat thanks for the ping, I'm checking deeper... but AFAIR we don't resolve env var during prepending phase before, it's done during the merging process (aka loadExtension) when all configs were merged. |
MatTheCat commentedMay 17, 2024
@yceruto thanks for looking! Not sure what you call “resolving” env vars but this PR essentially stringify |
yceruto commentedMay 17, 2024
Nevemind, I got it, it's about processing the configurator as beforehttps://github.com/symfony/symfony/pull/52843/files#diff-1cd56b329433fc34d950d6eeab9600752aa84a76cbe0693d3fab57fed0f547d3L150 |
…ig loader (yceruto)This PR was merged into the 7.1 branch.Discussion----------[DependencyInjection] Fix prepending strategy for php config loader| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#54852| License | MITworking towards fixing#54970Commits-------a7ecb85 Fix prepending strategy for php config loader
…ig loader (yceruto)This PR was merged into the 7.1 branch.Discussion----------[DependencyInjection] Fix prepending strategy for php config loader| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | Fix #54852| License | MITworking towards fixingsymfony/symfony#54970Commits-------a7ecb8568d Fix prepending strategy for php config loader
Uh oh!
There was an error while loading.Please reload this page.
Since#52843,
ContainerConfigurator::extensionis not called anymore which meansParamConfigurators are no longer processed and converted to strings, and makes nodes validation fail: e.g.framework.secretwould receive anEnvConfiguratorand throw because it is not a scalar.