Skip to content

[DependencyInjection] #[Autowire] attribute should have precedence over bindings #51559

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

Merged

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Sep 4, 2023

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #49082
License MIT
Doc PR -

It seems logical (I think) that the #[Autowire] attribute has precedence over any bindings.

@ruudk
Copy link
Contributor

ruudk commented Nov 30, 2023

Thanks for fixing this 🙏 . Could you rebase this?

@HypeMC HypeMC force-pushed the fix-autowire-attribute-with-bindings branch from b9bacf2 to 57f4b25 Compare November 30, 2023 15:29
@stof
Copy link
Member

stof commented Nov 30, 2023

@nicolas-grekas is it right to have to make ResolveBindingPass aware of the Autowire attribute ? Shouldn't this be handled by the AutowirePass ?

@nicolas-grekas
Copy link
Member

I agree, the bindings pass has no reasons to know about autowiring stuff.
Can't we fix this by changing the order of the two passes?

@HypeMC
Copy link
Contributor Author

HypeMC commented Nov 30, 2023

@nicolas-grekas @stof My initial approach was to fix it in AutowirePass. I don't remember the exact reason, but I couldn't do it like that so I went with this instead.

I can try to change the order of the passes, but I'm worried that might break other stuff. I'll try it sometime today or tomorrow.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 30, 2023

It's also not obvious to me what we want here.

What this PR achieves is this precedence, isn't it?

  1. autowiring by attribute
  2. binding by type+name of argument
  3. binding by type only
  4. autowiring by type+name of argument
  5. autowiring by type only

Dunno if we should account for that but bindings can also be defined by explicit configuration and/or by autoconfiguration.

Dunno how we can make AutowirePass achieve this ordering (or another) given that once bindings are resolved, we don't know if some argument's values came through bindings or via another path (explicit/auto config).

Maybe this approach is the most pragmatic.

WDYT?

@HypeMC HypeMC force-pushed the fix-autowire-attribute-with-bindings branch from 57f4b25 to c429fe5 Compare December 1, 2023 20:36
@nicolas-grekas
Copy link
Member

Any other opinion here? @stof?

@nicolas-grekas
Copy link
Member

Thank you @HypeMC.

@nicolas-grekas nicolas-grekas merged commit eb32f49 into symfony:6.3 Jan 30, 2024
@HypeMC HypeMC deleted the fix-autowire-attribute-with-bindings branch January 30, 2024 09:52
This was referenced Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants