Page MenuHomePhabricator
Create Task
Maniphest T13303

There is an extra or missing "array_reverse()" somewhere after recent transaction strength sorting changes
Closed, ResolvedPublic
Actions

Description

Symptoms:

  • Maniphest email is now showing transactions in reverse order (not sure this was up to date, though).
  • When a revision is accepted, the notification popup bubble now says "X is ready to land", not "alice accepted X".

Probable cause:

  • The "stablize sort in PHP 7" stuff probably missed an extra array_reverse() somewhere, since the old sort order was flipped? Or the new sort order is flipped?

Revisions and Commits

rPHU libphutil
D20611rPHUb675a06bf741 (stable) Allow "PhutilClassMapQuery" to sort with a PhutilSortVector sort…
D20611rPHUf5879cc6a9dc Allow "PhutilClassMapQuery" to sort with a PhutilSortVector sort function
D20582rPHUada64585035e Consider "msort()" on "PhutilSortVector" objects to be an error
rARC Arcanist
D20605rARCd92fa96366c0 Fix two "msort()" vs "msortv()" issues in "arc land"
rP Phabricator
ClosedD20587 Fix more "msort()" vs "msortv()" callsites
D20585rP2bc045bab8d7 Fix another stray "msort()/msortv()" issue
D20583rP874282db7589 Correct "msort()" vs "msortv()" to more fully stabilize transaction sorts after…
D20563rP81134d7e7d54 After reloading transactions for the recipient while building transaction mail…

Related Objects

Event Timeline

epriestley triaged this task as Low priority.May 30 2019, 7:09 PM
epriestley created this task.
Herald added a subscriber: amckinley. · View Herald TranscriptMay 30 2019, 7:09 PM
epriestley renamed this task from There is an extra or missing "array_reverse()" somewhere after recent transaction strength sorting changs to There is an extra or missing "array_reverse()" somewhere after recent transaction strength sorting changes.May 30 2019, 7:09 PM
epriestley added a revision: D20563: After reloading transactions for the recipient while building transaction mail, put them in the input order.May 30 2019, 10:37 PM
epriestley added a comment.May 30 2019, 11:15 PM

D20563 probably fixes mail. I don't think (?) it will fix notifications but haven't hunted that down yet.

epriestley added a commit: rP81134d7e7d54: After reloading transactions for the recipient while building transaction mail….May 31 2019, 12:15 AM
epriestley added a comment.Jun 17 2019, 5:23 PM

This is doing somewhat better now, but I've still seen:

  • Notifications for accepted commits are saying "Dxxx is ready to land", not "alice accepted X".
  • Task creation email sent out with [ Changed Policy ] or something instead of [ Created ].

Pretty sure these were both after rP81134d7e made it to secure so there's at least some more stuff to hunt down.

epriestley added a comment.Jun 17 2019, 5:33 PM

uhoh looks like I'm a dum dum

epriestley added a revision: D20582: Consider "msort()" on "PhutilSortVector" objects to be an error.Jun 17 2019, 5:36 PM
epriestley added a revision: D20583: Correct "msort()" vs "msortv()" to more fully stabilize transaction sorts after recent changes.Jun 17 2019, 5:40 PM
epriestley added a commit: rPHUada64585035e: Consider "msort()" on "PhutilSortVector" objects to be an error.Jun 17 2019, 8:00 PM
epriestley added a commit: rP874282db7589: Correct "msort()" vs "msortv()" to more fully stabilize transaction sorts after….Jun 17 2019, 8:07 PM
epriestley added a revision: D20585: Fix another stray "msort()/msortv()" issue.Jun 17 2019, 8:11 PM
epriestley added a commit: rP2bc045bab8d7: Fix another stray "msort()/msortv()" issue.Jun 17 2019, 8:20 PM
epriestley added a comment.Jun 17 2019, 9:25 PM

I deployed that last round of things to secure. Not totally confident I got everything, but hopefully we're in better shape now.

epriestley closed this task as Resolved.Jun 19 2019, 5:29 PM
epriestley claimed this task.
epriestley added a revision: D20587: Fix more "msort()" vs "msortv()" callsites.
epriestley removed a subscriber: epriestley.
epriestley added a subscriber: epriestley.

I think that might be everything? Not entirely sure, but haven't seen any more since the last deploy.

epriestley added a revision: D20605: Fix two "msort()" vs "msortv()" issues in "arc land".Jun 20 2019, 9:41 PM
epriestley added a commit: rARCd92fa96366c0: Fix two "msort()" vs "msortv()" issues in "arc land".Jun 20 2019, 11:07 PM
epriestley added a revision: D20611: Allow "PhutilClassMapQuery" to sort with a PhutilSortVector sort function.Jun 22 2019, 4:03 PM
epriestley added a commit: rPHUf5879cc6a9dc: Allow "PhutilClassMapQuery" to sort with a PhutilSortVector sort function.Jun 22 2019, 4:05 PM
epriestley added a commit: rPHUb675a06bf741: (stable) Allow "PhutilClassMapQuery" to sort with a PhutilSortVector sort….
Phabricator · Built by Phacility