Skip to content

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Oct 3, 2023

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

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

@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:

* 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 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
…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]`
@@ -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

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.

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


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

@bendavies bendavies Oct 12, 2023

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?

This was referenced Oct 21, 2023
fabpot added a commit that referenced this pull request May 2, 2024
…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]
fabpot added a commit that referenced this pull request Jun 20, 2025
…#[TaggedLocator]` attributes (GromNaN)

This PR was merged into the 8.0 branch.

Discussion
----------

[DependencyInjection] Remove `#[TaggedIterator]` and `#[TaggedLocator]` attributes

| Q             | A
| ------------- | ---
| Branch?       | 8.0
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | -
| License       | MIT

The new attributes `#[AutowireLocator]` and `#[AutowireIterator]` were introduced in Symfony 6.4 by #51392 and #51832.
These replace the previous attributes `#[TaggedIterator]` and `#[TaggedLocator]` which were introduced in Symfony 5.4 by #40406 and subsequently deprecated in 7.1 by #54371

Commits
-------

c096714 Remove TaggedIterator and TaggedLocator attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants