Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

@LANGERGabrielle
Copy link
Contributor

@LANGERGabrielleLANGERGabrielle commentedMar 3, 2022
edited
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets
LicenseMIT
Doc PR

Description

Runningcontainer:lint when a service uses#[Autoconfigure(binds: ['$myDependency' => {...})] raises an error :

[ERROR] A binding is configured for an argument named "$myDependency" under                  "_instanceof" in file  "/{...}/MyService.php", but no             corresponding argument has been found. It may be unused and should be           removed, or it may have a typo.

How to reproduce

Given a service

#[Autoconfigure(bind: ['$myDependency' =>'@app.my_dependency'])]class MyService{publicfunction__construct(MyDependency$myDependency)    {    }}

@app.my_dependency is correctly injected intoMyService. But when runningcontainer:lintwith the container dumped (App_KernelDevDebugContainer.xml is present and up to date), it raises an error.

This only happens with#[Autoconfigure]. A similar configuration but usingservices.yaml works as expected :

__instanceof:App\MyService:binds:$myDependency:'@app.my_dependency'

Explanation of the issue

  1. RegisterAutoconfigureAttributesPass parses the#[Autoconfigure] and registers its binding.
  2. ResolveBindingsPass reads the binding and uses it to configure the servicearguments.
  3. The container is dumped, withApp_KernelDevDebugContainer.xml containing
    <serviceid="App\MyService"class="App\MyService"autowire="true"autoconfigure="true">  <argumenttype="service"id="app.my_dependency"/></service>
  4. ContainerLintCommand then creates a container from the dump.
  5. The container is compiled. SinceKernel::initializeBundles() is not called, only thebase passes are used.
  6. RegisterAutoconfigureAttributesPass parses the#[Autoconfigure] and registers its binding,again.
  7. ResolveBindingsPass sees 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.
  8. The error is then thrown.

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 toDependencyInjection.

Both fixes have been tested on a mid size Symfony application.

@carsonbotcarsonbot added this to the5.4 milestoneMar 3, 2022
@LANGERGabrielleLANGERGabrielleforce-pushed thefix-autoconfigure-attribute-container-lint branch 2 times, most recently fromb734a71 todc961e2CompareMarch 3, 2022 21:51
@LANGERGabrielleLANGERGabrielleforce-pushed thefix-autoconfigure-attribute-container-lint branch fromdc961e2 to2e72612CompareMarch 3, 2022 22:05
@LANGERGabrielleLANGERGabrielle marked this pull request as draftMarch 3, 2022 22:14
@LANGERGabrielleLANGERGabrielleforce-pushed thefix-autoconfigure-attribute-container-lint branch 2 times, most recently from002daff to209516bCompareMarch 3, 2022 22:31
@LANGERGabrielleLANGERGabrielle marked this pull request as ready for reviewMarch 3, 2022 22:42
@LANGERGabrielleLANGERGabrielle changed the title[FrameworkBundle] Fix container:lint and #[Autoconfigure(binds: ...)] failing[FrameworkBundle][DependencyInjection] Fix container:lint and #[Autoconfigure(binds: ...)] failingMar 3, 2022
@nicolas-grekas
Copy link
Member

Thanks for the description of the issue.

As a rule of thumb, introducing a new method is a hint for a new feature.
I'd really prefer not adding any new method to fix the issue, because from an API pov, it makes things more complex.

Can you give the simpler solution a try instead please?

@LANGERGabrielleLANGERGabrielleforce-pushed thefix-autoconfigure-attribute-container-lint branch from209516b to88017ffCompareMarch 4, 2022 17:34
@LANGERGabrielleLANGERGabrielle changed the title[FrameworkBundle][DependencyInjection] Fix container:lint and #[Autoconfigure(binds: ...)] failing[FrameworkBundle] Fix container:lint and #[Autoconfigure(binds: ...)] failingMar 4, 2022
@LANGERGabrielle
Copy link
ContributorAuthor

LANGERGabrielle commentedMar 4, 2022
edited
Loading

@nicolas-grekas done.
If you'd like, I can open a PR for the more future proof fix against 6.1.

@LANGERGabrielleLANGERGabrielleforce-pushed thefix-autoconfigure-attribute-container-lint branch from88017ff to309998bCompareMarch 4, 2022 17:48
Copy link
Member

@nicolas-grekasnicolas-grekas left a 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
Copy link
ContributorAuthor

LANGERGabrielle commentedMar 4, 2022
edited
Loading

CurrentlyContainerLintCommand is not tested in any way.
I could add a test, not sure which level would be best (I like full integration tests but that might not be the best).
I tested this manually quickly on a mid size application, and it succeeds.

@LANGERGabrielle
Copy link
ContributorAuthor

This seems related to#30930

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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 ;)

LANGERGabrielle reacted with thumbs up emoji
@nicolas-grekas
Copy link
Member

Thank you@LANGERGabrielle.

@nicolas-grekasnicolas-grekas merged commit5f1e0c8 intosymfony:5.4Mar 8, 2022
@LANGERGabrielleLANGERGabrielle deleted the fix-autoconfigure-attribute-container-lint branchMarch 8, 2022 16:41
@fabpotfabpot mentioned this pull requestApr 2, 2022
@fabpotfabpot mentioned this pull requestApr 2, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

3 participants

@LANGERGabrielle@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp