[Doctrine Bridge] return 0 on equality when sorting listeners/subscribers #27126
Conversation
| @@ -66,6 +66,10 @@ public function process(ContainerBuilder $container) | |||
| $a = isset($a['priority']) ? $a['priority'] : 0; | |||
| $b = isset($b['priority']) ? $b['priority'] : 0; | |||
|
|
|||
| if ($a === $b) { | |||
| return 0; | |||
There was a problem hiding this comment.
Let's return $b - $a and remove both the if and the ternary operator.
|
@nicolas-grekas @stof now that I see this PR here I remembered I also worked on this a while ago: What exactly happened to this change? 😄 Was it reverted somehow?
|
|
Ok I think it got lost during this merge 😢 dc66960#diff-27d2e9b071d766df504c3fe4131e7abf So it never propagated to any higher branch than 2.8. What to do? |
|
@ostrolucky bug fix. It doesn't make sense to return a non-zero for equal values. Sorting algorithms have undefined behavior when doing so. |
|
@dmaicher are you fine with this PR? If not, can you please advise? Or would you like to submit another PR if the change required is too big? |
|
@nicolas-grekas well the thing is that #21977 now effectively is not fixed anymore on 3.4+ 😕 So I would propose to port the changes I made for 2.8 with a new PR into 3.4. I think this might also solve the sorting issue fixed here as I used the "stable sort" approach that is also used elsewhere when sorting tagged services by priority. I will prepare a new PR today and then @bendavies can you verify it also solves your sorting issue? |
|
here is the new PR: #27133 @bendavies can you verify that your problems are also solved by it? |
|
Closing in favor of #27133 |
…s (dmaicher) This PR was merged into the 3.4 branch. Discussion ---------- [Doctrine Bridge] fix priority for doctrine event listeners | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21977 | License | MIT | Doc PR | - As discussed in #27126 this ports changes from #22001 to 3.4 that were dropped when merging 2.8 into 3.2 here: dc66960#diff-27d2e9b071d766df504c3fe4131e7abf I took my original changeset from 2.8 and applied all commits since then on top of that. Commits ------- b3ac938 [Doctrine Bridge] fix priority for doctrine event listeners
In our app (simplified), we have two event subscribers,
AandB.The app relies on
Arunning beforeB, but we had mistakenly left their priorities as the default,0. By luck/chance, they were sorted such thatAsorted beforeB. All was well.Then all of a sudden, they started firing in the 'wrong' order,
BthenA.Debugging this, it was because we added a third event listener,
C, which caused the order ofAandBto be reversed.This can be seen here:
https://3v4l.org/SQefd
Note that the order of
aandbis reversed oncecis added.Adding an equality check (mimicking the spaceship operator) "fixes" the issue (even thought the behavior of
uasortis undefined), and makes the results consistent, I hope?Now, clearly it was our fault for not defining priorities, but we may have been alerted to the error of our ways if the ordering returned from the sort was consistent in the first place.