8000 [DependencyInjection] Exclude referencing service (self) in `TaggedIteratorArgument` by chalasr · Pull Request #48685 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

Merged
fabpot merged 1 commit intosymfony:6.3from
chalasr:tagged-autoexclude-self
Jan 5, 2023
Merged

[DependencyInjection] Exclude referencing service (self) in TaggedIteratorArgument#48685
fabpot merged 1 commit intosymfony:6.3from
chalasr:tagged-autoexclude-self

Conversation

@chalasr
Copy link
Member
@chalasr chalasr commented Dec 16, 2022 •
edited
Loading
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 December 16, 2022 17:50
OskarStark
OskarStark reviewed Dec 16, 2022
View reviewed changes
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

stof
stof requested changes Dec 16, 2022
View reviewed changes
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
OskarStark commented Dec 16, 2022

Friendly ping @lyrixx

@chalasr
Copy link
Member Author
chalasr commented Dec 16, 2022 •
edited
Loading

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 December 16, 2022 18:17
@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 December 22, 2022 17:17
@chalasr
Copy link
Member Author
8000 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 December 22, 2022 18:30
@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 December 22, 2022 18:31
@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 requested a review from dunglas as a code owner 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
@chalasr chalasr force-pushed the tagged-autoexclude-self branch from 49f0d8a to 1cadd46 Compare December 29, 2022 11:16
@fabpot
Copy link
Member
fabpot commented Jan 5, 2023

Thank you @chalasr.

@fabpot fabpot mentioned this pull request Jan 5, 2023
Closed
@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
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 26, 2023
Merged
@fabpot fabpot mentioned this pull request May 1, 2023
Merged
@HypeMC HypeMC mentioned this pull request May 16, 2023
Merged
nicolas-grekas added a commit that referenced this pull request May 18, 2023
@nicolas-grekas
bug #50351 [DependencyInjection] Add excludeSelf option to dumpers …
3d6e0bd 
…(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

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof requested changes

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

+1 more reviewer

@maxhelias maxhelias maxhelias approved these changes

Reviewers whose approvals may not affect merge requirements

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.

7 participants

@chalasr @OskarStark @stof @nicolas-grekas @fabpot @maxhelias @carsonbot
0