Skip to content
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

[DependencyInjection] Clarify the #[Target] attribute #19789

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Apr 14, 2024

The #[Target] attribute seems to be a constant source of confusion for developers, as evident by:

Also, the example given is either unclear or just wrong. Hopefully, this helps clarify things.

@javaDeveloperKid
Copy link
Contributor

javaDeveloperKid commented Apr 14, 2024

Good clarification. I've never understood how this attribute works 😄 I had two interpretations but both wrong.

Btw. I think this should also be clarified in the source code above $name constructor parameter doc block and class doc block (the latter is a little better but still can be improved). https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/DependencyInjection/Attribute/Target.php

@HypeMC HypeMC force-pushed the clarify-the-target-attribute branch from 210f1a5 to 06d88b6 Compare April 15, 2024 09:16
@OskarStark
Copy link
Contributor

@nicolas-grekas a final review from your side would be helpful, thanks

@HypeMC HypeMC force-pushed the clarify-the-target-attribute branch from 06d88b6 to 2fb1ada Compare April 19, 2024 18:53
@nicolas-grekas
Copy link
Member

LGTM but maybe we should tell about the debug:autowiring command, which does report the target name since symfony/symfony#50718?

@HypeMC
Copy link
Contributor Author

HypeMC commented Apr 22, 2024

@nicolas-grekas Good idea, I've created a followup PR since this one targets 5.4, and the feature was introduced in 6.4.

@javiereguiluz javiereguiluz merged commit 59d5b18 into symfony:5.4 Jun 24, 2024
3 checks passed
@javiereguiluz
Copy link
Member

@HypeMC thanks for this contribution and sorry it took us so long to merge it.

@javiereguiluz
Copy link
Member

@HypeMC I upmerged this in 6.4, 7.0, 7.1 and 7.2 branches.

I'm not sure I did everything right, because in 6.4 we show both upperTransformer and shoutyTransformer. I don't know if both are correct or the "shouty" thing is a leftover from 5.4 branch and 6.4 branch was wrong before merging this PR.

If you can, please review it. See ad1e3b2

@HypeMC HypeMC deleted the clarify-the-target-attribute branch June 24, 2024 09:42
javiereguiluz added a commit that referenced this pull request Jun 24, 2024
…2 (HypeMC)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[DependencyInjection] Clarify the `#[Target]` attribute 2

Follow-up to #19789, adds a [paragraph](bd00116) about the `debug:autowiring` command.

Commits
-------

81cc7f0 [DependencyInjection] Clarify the `#[Target]` attribute 2
@HypeMC
Copy link
Contributor Author

HypeMC commented Jun 24, 2024

@javiereguiluz Yep, everything looks ok, I see you've also merged the follow up PR.

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.

None yet

6 participants