-
-
Notifications
You must be signed in to change notification settings - Fork 9.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 our terms of service and privacy 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
Conversation
b734a71 to
dc961e2
Compare
src/Symfony/Bundle/FrameworkBundle/Command/BuildDebugContainerTrait.php
Outdated
Show resolved
Hide resolved
dc961e2 to
2e72612
Compare
002daff to
209516b
Compare
|
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 to
88017ff
Compare
|
@nicolas-grekas done. |
…en it's loaded from the debug dump
88017ff to
309998b
Compare
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.
|
Currently |
|
This seems related to #30930 |
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 ;)
|
Thank you @LANGERGabrielle. |
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 the base 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 is not 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 be 209516b, but it would require changes to
DependencyInjection.Both fixes have been tested on a mid size Symfony application.