Skip to content

[DependencyInjection] Add #[AutowireIterator] attribute and improve #[AutowireLocator] #51832

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 6, 2023
Merged

[DependencyInjection] Add #[AutowireIterator] attribute and improve #[AutowireLocator] #51832

merged 2 commits into from
Oct 6, 2023

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Oct 3, 2023 •
edited
Loading

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT

This PR is building on #51392 to add an #[AutowireIterator] attribute and improve #[AutowireLocator].

The new #[AutowireIterator] attribute can be used to describe what #[AutowireLocator] can do, except that we get an iterator instead of a container.

And #[AutowireLocator] can now be used instead of #[TaggedLocator]: #[AutowireLocator('foo')] and done.

In order to describe that you want a list of services, we cannot use named arguments anymore so we have to pass an array now: #[AutowireLocator(['foo' => 'F', 'bar' => 'B'])] should be used instead of #[AutowireLocator(foo: 'F', bar: 'B')].

Last but not least, this adds support for nesting SubscribedService objects in the list of described services. This provides feature-parity with what we can do when implementing ServiceSubscriberInterface.

I didn't deprecate TaggedIterator nor TaggedLocator. We could, but maybe we should wait for 7.1?

TODO:

PS: while writing this, I realize that we may merge both tags in one, and let AutowirePass decide if it should build a locator or an iterator based on the type of the argument that has the attribute. We'd "just" need to find a name that'd work for that.

@carsonbot carsonbot added this to the 6.4 milestone Oct 3, 2023
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 3, 2023
@nicolas-grekas nicolas-grekas force-pushed the di-autowire-tagged branch 2 times, most recently from 17a9a15 to 8ed9318 Compare October 4, 2023 15:22
@nicolas-grekas
Copy link
Member Author

PR updated and ready. Big Hurray to @kbond for the tests 🙏 🤗

@KerianMM
Copy link
Contributor

KerianMM commented Oct 5, 2023

TaggedIterator and TaggedLocator attributes are they still use full?

@nicolas-grekas
Copy link
Member Author

@KerianMM that's an open question. I see no hurry in deprecating them, I'd be fine waiting for 7.1.

@nicolas-grekas nicolas-grekas force-pushed the di-autowire-tagged branch 2 times, most recently from 7bb9a9e to 38c48ef Compare October 5, 2023 13:34
@stof
Copy link
Member

stof commented Oct 5, 2023

Why not improving the existing attributes instead of adding new ones ?

@nicolas-grekas
Copy link
Member Author

Why not improving the existing attributes instead of adding new ones ?

Because I think it's important to push the Autowire* convention. It helps discovering/remembering those autowiring-related attributes.

@chalasr
Copy link
Member

chalasr commented Oct 5, 2023 •
edited
Loading

I didn't deprecate TaggedIterator nor TaggedLocator. We could, but maybe we should wait for 7.1?

👍 to do it on 7.1 and give time to people having them since 5.3. The docs should probably recommend the new ones asap though

@OskarStark
Copy link
Contributor

I updated the syntax for #[AutowireLocator]:

@OskarStark OskarStark mentioned this pull request Oct 6, 2023
Merged
@OskarStark
Copy link
Contributor

Last but not least, this adds support for nesting SubscribedService objects in the list of described services. This provides feature-parity with what we can do when implementing ServiceSubscriberInterface.

Can you add a code example please? Thanks

@nicolas-grekas
Copy link
Member Author

What's in ServiceSubscriberInterface:

symfony/src/Symfony/Contracts/Service/ServiceSubscriberInterface.php

Lines 53 to 57 in d27190a

* additionally, an array of {@see SubscribedService}'s can be returned:
*
* * [new SubscribedService('logger', Psr\Log\LoggerInterface::class)]
* * [new SubscribedService(type: Psr\Log\LoggerInterface::class, nullable: true)]
* * [new SubscribedService('http_client', HttpClientInterface::class, attributes: new Target('githubApi'))]

@OskarStark
Copy link
Contributor

Updated

@nicolas-grekas
Copy link
Member Author

Thank you @kbond.

@nicolas-grekas nicolas-grekas merged commit 95fa158 into symfony:6.4 Oct 6, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 6, 2023
Closed
@nicolas-grekas nicolas-grekas deleted the di-autowire-tagged branch October 6, 2023 09:42
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Oct 10, 2023
@OskarStark
feature #18993 [DependencyInjection] Update syntax for `#[AutowireLoc…
4918230 
…ator]` and add `#[AutowireIterator]` (OskarStark)

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

Discussion
----------

[DependencyInjection] Update  syntax for `#[AutowireLocator]` and add `#[AutowireIterator]`

cc `@kbond`

Waiting code merge
* symfony/symfony#51832

Fixes #18997

Commits
-------

528cbda [DependencyInjection] Update  syntax for `#[AutowireLocator]` and add `#[AutowireIterator]`
bendavies
bendavies reviewed Oct 11, 2023
View reviewed changes
src/Symfony/Component/HttpKernel/composer.json
@@ -63,6 +63,7 @@
"symfony/http-client-contracts": "<2.5",
"symfony/mailer": "<5.4",
"symfony/messenger": "<5.4",
"symfony/service-contracts": "<3.2",
Copy link
Contributor

@bendavies bendavies Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbond why was this added? is there a way to remove this conflict?

this makes it not possible for us to upgrade to symfony 6.4 because this forces a requirement for psr/container ^2.0, and we are still on v1 because the latest version of laminas/laminas-servicemanager requires v1:
https://github.com/laminas/laminas-servicemanager/blob/3.23.x/composer.json#L26

this is sort of the same issue as api-platform/core#5811

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @nicolas-grekas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @kbond, tagged you as it looked like your commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed because the test case uses the $type argument of SubscribedService, which exists since 3.2.
PR welcome to change the test case and relax this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

bendavies
bendavies reviewed Oct 12, 2023
View reviewed changes
src/Symfony/Component/DependencyInjection/Attribute/AutowireIterator.php

if ($type instanceof SubscribedService) {
$key = $type->key ?? $key;
$attributes = $type->attributes;
Copy link
Contributor

@bendavies bendavies Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symfony/dependency-injection requires "symfony/service-contracts": "^2.5|^3.0, but attributes, nullable and type are not defined on SubscribedService in until 3.2.
is this a mistake?

@xabbuh xabbuh mentioned this pull request Oct 13, 2023
Closed
This was referenced Oct 21, 2023
Merged
Merged
@bendavies bendavies mentioned this pull request Dec 19, 2023
Merged
@GromNaN GromNaN mentioned this pull request Mar 21, 2024
Merged
fabpot added a commit that referenced this pull request May 2, 2024
@fabpot
feature #54371 [DependencyInjection] Deprecate #[TaggedIterator] an…
0302e1d 
…d `#[TaggedLocator]` (GromNaN)

This PR was merged into the 7.1 branch.

Discussion
----------

[DependencyInjection] Deprecate `#[TaggedIterator]` and `#[TaggedLocator]`

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        | Planned in #51832
| License       | MIT

`#[TaggedIterator]` and `#[TaggedLocator]` attributes are replaced by `#[AutowireIterator]` and `#[AutowireLocator]`, for naming consistency with all `#[Autowire*]` attributes.

Commits
-------

4bc6bed Deprecate #[TaggedIterator] and #[TaggedLocator]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond kbond approved these changes

@GromNaN GromNaN GromNaN left review comments

@stof stof stof left review comments

@bendavies bendavies bendavies left review comments

@weaverryan weaverryan weaverryan approved these changes

@chalasr chalasr chalasr approved these changes

@dunglas dunglas Awaiting requested review from dunglas

Assignees
No one assigned
Labels
DependencyInjection Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
10 participants
@nicolas-grekas @KerianMM @stof @chalasr @OskarStark @weaverryan @kbond @GromNaN @bendavies @carsonbot