Prevent double registrations related to tag priorities #22396
Conversation
6df5ea8 to
8822411
Compare
There was a problem hiding this comment.
Some of the problems that I talk about at the bottom of my description in #22234 are not actually a problem in 2.7 - but only become a problem in 3.2 when PriorityTaggedServiceTrait is added. For example, here, if you add 2 security.voter tags, it only registers the voter 1 time. I think that logic is perfect. But when AddSecurityVoterPass started using PriorityTaggedServiceTrait, that changed: the voter started being registered multiple times for multiple tags. So there may be a few things to fix in 2.7, but a lot of the fixes are on 3.2.
| $subscriberTag = $this->tagPrefix.'.event_subscriber'; | ||
| $taggedSubscribers = $this->findAndSortTags($subscriberTag, $container); | ||
|
|
||
| foreach ($taggedSubscribers as $taggedSubscriber) { | ||
| $id = $taggedSubscriber[0]; | ||
| if (isset($seen[$id = $taggedSubscriber[0]])) { |
There was a problem hiding this comment.
You know me, I love explaining things with comments, but your call :p
> if a service is tagged twice, avoid adding the subscriber twice| if (isset($seen[$tag['event']][$id])) { | ||
| continue; | ||
| } | ||
| $seen[$tag['event']][$id] = true; |
There was a problem hiding this comment.
I think we should maybe leave this one as it is. At the very least, it's valid to have 2 identical tags, each with a different connection option.
| if (isset($tag['priority'])) { | ||
| $priority = $tag['priority']; | ||
| break; | ||
| } |
There was a problem hiding this comment.
I think we should not change this one. For example:
services:
_instanceof:
Symfony\Component\Security\Core\Authorization\Voter\VoterInterface:
tags:
# you probably shouldn't set priority here, but let's pretend we did
- { name: security.voter, priority: 100 }
AppBundle\Security\CoolPersonVoter:
tags:
- { name: security.voter }In the final Definition, the tags will appear in this order:
- security.voter (no
priority) - security.voter,
priority100
So, what's the desired priority? I think it's 0 - you're overriding the priority by setting it on the service. So, I think taking the priority off of the first tag is correct.
| $priority = $tag['priority']; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same here, I think it was correct before
| } | ||
| $sortedServices[$priority][] = new Reference($serviceId); |
There was a problem hiding this comment.
Again, I think taking the priority from the index 0 tag is correct. But this fixes registering the service 2 times for 2 tags.
| $priority = $tag['priority']; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same here, I think we should use the 0 index's priority
8822411 to
384b92d
Compare
|
👍 This looks like it handled all the tag problems on the 2.7 branch. |
384b92d to
6764dcd
Compare
|
Thank you @nicolas-grekas. |
…colas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- Prevent double registrations related to tag priorities | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - The current logic is inconsistent, and allows the same id to be used several times. This makes the first explicit priority to always win. Commits ------- 6764dcd Prevent double registrations related to tag priorities
…es (nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [2.8] Prevent double registrations related to tag priorities | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - #22396 on 2.8 Commits ------- 2be5821 [2.8] Prevent double registrations related to tag priorities
…es (nicolas-grekas) This PR was merged into the 3.2 branch. Discussion ---------- [3.2] Prevent double registrations related to tag priorities | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - #22396 and #22399 on 3.2 Commits ------- ec6a2f9 [3.2] Prevent double registrations related to tag prioritie 8000 s
…show up first (weaverryan) This PR was merged into the 3.3-dev branch. Discussion ---------- Enhancing integration test to show that "override" tags show up first | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | not needed Relates a bit to #22396, in that I'm clarifying and emphasizing the following types of situations: ```yml services: _instanceof: Symfony\Component\Security\Core\Authorization\Voter\VoterInterface: tags: # you probably shouldn't set priority here, but let's pretend we did - { name: security.voter, priority: 100 } AppBundle\Security\CoolPersonVoter: tags: - { name: security.voter, priority: 50 } ``` In the final `Definition`, the tags will appear in this order: * security.voter, priority 50 * security.voter, priority 100 It works the same for parent-child definitions. tl;dr; If a service has the same tag multiple times, the one that should be used should appear *earlier* in the Definition. The code already works that way - this test emphasizes it. Commits ------- e9b96e5 Enhancing integration test to show that "override" tags always show up first
The current logic is inconsistent, and allows the same id to be used several times. This makes the first explicit priority to always win.