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

Jump to bottom

[DependencyInjection] Exclude referencing service (self) in TaggedIteratorArgument #48685

Merged
merged 1 commit into from Jan 5, 2023
Merged

[DependencyInjection] Exclude referencing service (self) in TaggedIteratorArgument #48685

merged 1 commit into from Jan 5, 2023
+60 −11

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Dec 16, 2022 •
edited

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

Suggested by @OskarStark in https://twitter.com/OskarStark/status/1594641457226436608?s=20&t=Wbqw_Mu2tMYQ0iouaG8yig.
This PR avoids the need to explicitly indicate to exclude the referencing service when using #[TaggedIterator] and variants.

Before:

final class DelegatingErrorTracker implements ErrorTracker
{
    public function __construct(
        #TaggedIterator(ErrorTracker::class, exclude: self::class)]
        private iterable $trackers
    ) {}
}

After:

final class DelegatingErrorTracker implements ErrorTracker
{
    public function __construct(
        #TaggedIterator(ErrorTracker::class]
        private iterable $trackers
    ) {}
}

I went with no deprecation as I think the breakage potential is close to zero, but happy to reconsider.

@carsonbot carsonbot added this to the 6.3 milestone Dec 16, 2022
@chalasr chalasr force-pushed the tagged-autoexclude-self branch from 5faa2d4 to e5570b4 Compare Dec 16, 2022
OskarStark
OskarStark reviewed Dec 16, 2022
View changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Uhh thanks for heading back 💪🏻 the good looks good as far as I can say

stof
stof requested changes Dec 16, 2022
View changes
Copy link
Member

@stof stof left a comment

this does not add support when creating such iterator arguments from Yaml or XML configs

@OskarStark
Copy link
Contributor

OskarStark commented Dec 16, 2022

Friendly ping @lyrixx

@chalasr
Copy link
Member Author

chalasr commented Dec 16, 2022 •
edited

this does not add support when creating such iterator arguments from Yaml or XML configs

ResolveTaggedIteratorArgumentPass covers any TaggedIteratorArgument no matter if it's wired by the yaml/xml/attribute file loader, isn't it?

@stof
Copy link
Member

stof commented Dec 16, 2022

@chalasr the missing part is not the resolution. It is the support for the new argument added in the constructor ($autoExcludeReferencingService) where Yaml and XML files are currently unable to control it.

@chalasr chalasr force-pushed the tagged-autoexclude-self branch from e5570b4 to 0cbb176 Compare Dec 16, 2022
@chalasr
Copy link
Member Author

chalasr commented Dec 16, 2022

Got it, thanks. Support added

@chalasr chalasr force-pushed the tagged-autoexclude-self branch from 0cbb176 to dd7c6dd Compare Dec 22, 2022
@chalasr
Copy link
Member Author

chalasr commented Dec 22, 2022

Rebased. Reviews welcome

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 22, 2022

So, this changes the current behavior, but we're confident this won't break anyone, right?

@chalasr chalasr force-pushed the tagged-autoexclude-self branch from dd7c6dd to 0a7138d Compare Dec 22, 2022
@chalasr chalasr changed the title [DependencyInjection] Auto exclude referencing service in TaggedIteratorArgument [DependencyInjection] Exclude referencing service (self) in TaggedIteratorArgument Dec 22, 2022
@chalasr chalasr force-pushed the tagged-autoexclude-self branch from 0a7138d to d357973 Compare Dec 22, 2022
@chalasr
Copy link
Member Author

chalasr commented Dec 22, 2022

Yes, at least I am :)

@chalasr chalasr force-pushed the tagged-autoexclude-self branch 2 times, most recently from 91d624c to a8afe94 Compare Dec 23, 2022
@chalasr chalasr requested a review from dunglas as a code owner Dec 23, 2022
@chalasr chalasr force-pushed the tagged-autoexclude-self branch 3 times, most recently from a8afe94 to 34a24ca Compare Dec 23, 2022
@chalasr
Copy link
Member Author

chalasr commented Dec 28, 2022

Anything else? :)

@chalasr chalasr force-pushed the tagged-autoexclude-self branch 3 times, most recently from 8b5f6fc to 49f0d8a Compare Dec 29, 2022
@chalasr chalasr force-pushed the tagged-autoexclude-self branch from 49f0d8a to 1cadd46 Compare Dec 29, 2022
fabpot
fabpot approved these changes Jan 5, 2023
View changes
@fabpot
Copy link
Member

fabpot commented Jan 5, 2023

Thank you @chalasr.

@fabpot fabpot mentioned this pull request Jan 5, 2023
Open
@fabpot fabpot merged commit a14eb6b into symfony:6.3 Jan 5, 2023
5 of 9 checks passed
@chalasr chalasr deleted the tagged-autoexclude-self branch Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas

@stof stof

@OskarStark OskarStark

@fabpot fabpot

@maxhelias maxhelias

@dunglas dunglas

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@chalasr @OskarStark @stof @nicolas-grekas @fabpot @maxhelias @carsonbot