Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Doctrine Bridge] return 0 on equality when sorting listeners/subscribers#27126
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Doctrine Bridge] return 0 on equality when sorting listeners/subscribers#27126
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $b =isset($b['priority']) ?$b['priority'] :0; | ||
| if ($a ===$b) { | ||
| return0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's return $b - $a and remove both the if and the ternary operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done!
dmaicher commentedMay 2, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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?
|
dmaicher commentedMay 2, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BC breakhttps://3v4l.org/O39FH
nicolas-grekas commentedMay 2, 2018
@ostrolucky bug fix. It doesn't make sense to return a non-zero for equal values. Sorting algorithms have undefined behavior when doing so. |
nicolas-grekas commentedMay 2, 2018
@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? |
dmaicher commentedMay 3, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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? |
dmaicher commentedMay 3, 2018
here is the new PR:#27133 @bendavies can you verify that your problems are also solved by it? |
nicolas-grekas commentedMay 3, 2018
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-27d2e9b071d766df504c3fe4131e7abfI 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
Uh oh!
There was an error while loading.Please reload this page.
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.