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] Exclude referencing service (self) in TaggedIteratorArgument #48685

Merged
merged 1 commit into from Jan 5, 2023

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Dec 16, 2022

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.

Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

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

@OskarStark
Copy link
Contributor

Friendly ping @lyrixx

@chalasr
Copy link
Member Author

chalasr commented Dec 16, 2022

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
Copy link
Member Author

chalasr commented Dec 16, 2022

Got it, thanks. Support added

@chalasr
Copy link
Member Author

chalasr commented Dec 22, 2022

Rebased. Reviews welcome

@nicolas-grekas
Copy link
Member

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

@chalasr chalasr changed the title [DependencyInjection] Auto exclude referencing service in TaggedIteratorArgument [DependencyInjection] Exclude referencing service (self) in TaggedIteratorArgument 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 December 23, 2022 12:48
@chalasr chalasr force-pushed the tagged-autoexclude-self branch 3 times, most recently from a8afe94 to 34a24ca Compare December 23, 2022 13:15
@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 December 29, 2022 11:08
@fabpot
Copy link
Member

fabpot commented Jan 5, 2023

Thank you @chalasr.

@fabpot fabpot merged commit a14eb6b into symfony:6.3 Jan 5, 2023
@chalasr chalasr deleted the tagged-autoexclude-self branch January 6, 2023 14:35
@fabpot fabpot mentioned this pull request May 1, 2023
nicolas-grekas added a commit that referenced this pull request May 18, 2023
…(HypeMC)

This PR was merged into the 6.3 branch.

Discussion
----------

[DependencyInjection] Add `excludeSelf` option to dumpers

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

Followup to #48685.

It seems that the `excludeSelf` option was not added to the dumpers or configuration functions. I don't think this was done intentionally, but was an oversight.

Commits
-------

09c7581 [DependencyInjection] Add exclude-self option to dumpers
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

7 participants