[DI] Don't track merged configs when the extension doesn't expose it #24021
Conversation
|
So, this happens when the DI extension doesn't call Shouldn't be common, so I wouldn't recommend a new release (yet.) |
|
@nicolas-grekas Thanks! I can confirm:
|
4d9342f to
a8e6aac
Compare
|
Hi guys, thanks for the fix! What about release that one since every bundle without the fix may have issue? @gharlan confirms that this PR fix the issue without change in the bundles. |
|
@jeremyFreeAgent if you have the issue (do you?), you'd better fix it in your bundle also because its current behavior will prevent correct tracking of ENV vars anyway. |
|
@nicolas-grekas I was not talking about my bundle but the community bundles. I mean okay we can PR every bundle we spot with that issue but what about basic users of Symfony that will only update Symfony and expect everything still works? BTW I have PR the fix in symfonycorp/connect-bundle#17 and It was merged :) |
|
I can confirm the same bug using 8p/guzzle-bundle. |
|
@samvdb can you submit a PR on https://github.com/8p/GuzzleBundle/blob/master/DependencyInjection/GuzzleExtension.php to fix that extension the same way as done in symfonycorp/connect-bundle#17? |
|
BTW, I'll propose to deprecate not calling |
|
@nicolas-grekas PR is open, can confirm this fixes the issue. |
|
@nicolas-grekas could we detect that this method was not called, and use the previous behavior for replacing env placeholders in this case ? This would not solve the shadowing of the config in case a variable is not used anymore in the end. But it would mean that the contain keeps working when the config is now shadowed. |
|
@stof see patch, that should be what L97 does. |
| // serialize config to catch env vars nested in object graphs | ||
| $config = serialize($extension->getProcessedConfigs()); | ||
| // serialize config and container to catch env vars nested in object graphs | ||
| $config = serialize($config).serialize($container->getDefinitions()).serialize($container->getAliases()).serialize($container->getParameterBag()->all()); |
There was a problem hiding this comment.
aliases don't need to be serialized. They only have a target and a public flag, and these cannot use parameters.
There was a problem hiding this comment.
|
Thank you @nicolas-grekas. |
… expose it (nicolas-grekas) This PR was merged into the 3.3 branch. Discussion ---------- [DI] Don't track merged configs when the extension doesn't expose it | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24020 | License | MIT | Doc PR | - This is driving me crazy :) Commits ------- a8e6aac [DI] Don't track merged configs when the extension doesn't expose it
This is driving me crazy :)