[DependencyInjection] Process PHP configs using the ContainerConfigurator #54970
Conversation
|
Thank you @MatTheCat. |
|
…that was fast! @yceruto does this PR seem okay to you? |
| foreach ($configBuilders as $configBuilder) { | ||
| $this->loadExtensionConfig($configBuilder->getExtensionAlias(), $configBuilder->toArray()); | ||
| $containerConfigurator->extension($configBuilder->getExtensionAlias(), $configBuilder->toArray(), $this->prepend); |
There was a problem hiding this comment.
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 is true, e.g. the nested importing might be broken...
There was a problem hiding this comment.
I saw you made the ContainerConfigurator 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 in 7c956d8
Don’t know if it is an issue at all though 😓
There was a problem hiding this comment.
Yes, it'll be an issue when want to prepend nested configs using imports
There was a problem hiding this comment.
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.
If you already have the tests I think you’re more reading than I am; go ahead 😁
Would ContainerConfiguration::extension serve a purpose then?
There was a problem hiding this comment.
Would ContainerConfiguration::extension serve a purpose then?
It should be avoided internally where recursive importing is involved, as now importing from prependExtension() 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 the prependExtension() or loadExtension() methods).
There was a problem hiding this comment.
Okaaay thanks; glad I asked your review 😄
|
@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. |
|
@yceruto thanks for looking! Not sure what you call “resolving” env vars but this PR essentially stringify |
Nevemind, I got it, it's about processing the configurator as before https://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 | MIT working towards fixing #54970 Commits ------- 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 | MIT working towards fixing symfony/symfony#54970 Commits ------- a7ecb8568d Fix prepending strategy for php config loader
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.