Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Fix container:lint and #[Autoconfigure(binds: ...)] failing#45629
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
[FrameworkBundle] Fix container:lint and #[Autoconfigure(binds: ...)] failing#45629
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b734a71 todc961e2Comparesrc/Symfony/Bundle/FrameworkBundle/Command/BuildDebugContainerTrait.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
dc961e2 to2e72612Compare002daff to209516bComparenicolas-grekas commentedMar 4, 2022
Thanks for the description of the issue. As a rule of thumb, introducing a new method is a hint for a new feature. Can you give the simpler solution a try instead please? |
209516b to88017ffCompareUh oh!
There was an error while loading.Please reload this page.
LANGERGabrielle commentedMar 4, 2022 • 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.
@nicolas-grekas done. |
…en it's loaded from the debug dump
88017ff to309998bCompare
nicolas-grekas left a comment
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.
LGTM. Do you think this would be tested somehow?
Also, we might want to make ContainerLintCommand use this trait. I'd be happy to do it if it's ok for the scope of this PR.
Not for this PR as that wouldn't qualify as a bugfix but sure on 6.1.
If you'd like, I can open a PR for the more future proof fix against 6.1.
No need to unless we have a use case. I'd keep things simple for now I think.
LANGERGabrielle commentedMar 4, 2022 • 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.
Currently |
LANGERGabrielle commentedMar 4, 2022
This seems related to#30930 |
nicolas-grekas left a comment
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.
Currently ContainerLintCommand is not tested in any way.
OK, I'm 👍 as is then.
If you want to add tests in another PR, it'll be always welcome ;)
nicolas-grekas commentedMar 8, 2022
Thank you@LANGERGabrielle. |
Uh oh!
There was an error while loading.Please reload this page.
Description
Running
container:lintwhen a service uses#[Autoconfigure(binds: ['$myDependency' => {...})]raises an error :How to reproduce
Given a service
@app.my_dependencyis correctly injected intoMyService. But when runningcontainer:lintwith the container dumped (App_KernelDevDebugContainer.xmlis present and up to date), it raises an error.This only happens with
#[Autoconfigure]. A similar configuration but usingservices.yamlworks as expected :Explanation of the issue
RegisterAutoconfigureAttributesPassparses the#[Autoconfigure]and registers its binding.ResolveBindingsPassreads the binding and uses it to configure the servicearguments.App_KernelDevDebugContainer.xmlcontainingContainerLintCommandthen creates a container from the dump.Kernel::initializeBundles()is not called, only thebase passes are used.RegisterAutoconfigureAttributesPassparses the#[Autoconfigure]and registers its binding,again.ResolveBindingsPasssees that the service already has anargument(from the xml dump from the firstResolveBindingsPass). This mean that the binding isnot used this time,and it is never removed from$unusedBindings.Explanation of the fix
This fix removes the passes that already processed the dumped container.
A more future proof fix would be209516b, but it would require changes to
DependencyInjection.Both fixes have been tested on a mid size Symfony application.