Prevent double registrations related to tag priorities #22396
Conversation
nicolas-grekas
force-pushed
the
mono-prio
branch
from
6df5ea8 to
8822411
Compare
weaverryan
left a comment
weaverryan
left a comment
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.
|
|
||
| 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
| break; | ||
| } | ||
| } | ||
| $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
nicolas-grekas
force-pushed
the
mono-prio
branch
from
8822411 to
384b92d
Compare
|
👍 This looks like it handled all the tag problems on the 2.7 branch. |
nicolas-grekas
force-pushed
the
mono-prio
branch
from
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 priorities
…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
3 participants
The current logic is inconsistent, and allows the same id to be used several times. This makes the first explicit priority to always win.