Page MenuHomePhabricator
Create Task
Maniphest T225977

Revision::getPrevious() returns null within onNewRevisionFromEditComplete hook
Open, LowPublic
Actions

Description

This ticket is to document the anomaly that caused T223828

Sometime in early May 2019, Revision::getPrevious() started returning null within the onNewRevisionFromEditComplete, even if a previous revision exists. This broke functionality in PageTriage. I have not confirmed this behaviour elsewhere in MediaWiki, but we were able to deduce nothing in PageTriage was the culprit.

Outside this context, Revision::getPrevious() works as expected. It seems to only be within onNewRevisionFromEditComplete. Changing the Core code to use READ_LATEST still does not resolve this issue, so I don't believe it is any sort of race condition.

A search shows the onNewRevisionFromEditComplete hook is not used many places and, for deployed WMF extensions, none are also using Revision::getPrevious(). Instead some (e.g. Wikibase) are using Revision::newFromId( $parentId ) to get the previous revision. Using this tactic worked for PageTriage, so we went with that.

The suspect patch I found was https://gerrit.wikimedia.org/r/c/mediawiki/core/+/507028 but even reverting it did not resolve the problem for me. I reverted back MediaWiki, and PageTriage, as far as I could (until I hit db changes), and I was still getting null for Revision::getPrevious(). So, the issue is truly a mystery!

Related Objects

Event Timeline

• MusikAnimal created this task.Jun 18 2019, 12:30 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 18 2019, 12:30 AM
• MusikAnimal added a subscriber: daniel.Jun 18 2019, 12:33 AM

@daniel Adding you as a subscriber, should you want to pursue this further. I did not tag this with Core Platform Team Initiatives (MCR) as I can't confirm it's truly MCR-related, but that is my best guess.

Regardless, thank you for the help you provided over at T223828#5236438 :)

• MusikAnimal updated the task description. (Show Details)Jun 18 2019, 12:33 AM
• MusikAnimal updated the task description. (Show Details)
Aklapper edited projects, added MediaWiki-Core-Revision-backend; removed MediaWiki-General.Jun 18 2019, 3:04 AM
• Krinkle moved this task from Untriaged to Revision on the MediaWiki-Core-Revision-backend board.Aug 1 2019, 7:56 PM
DannyS712 subscribed.Apr 27 2020, 4:07 AM

Suggest declining per T251073: Hard deprecate Revision::getPrevious

daniel edited projects, added Platform Engineering; removed Technical-Debt.Apr 27 2020, 10:04 AM

Tagging CPT for triage

daniel added a comment.Apr 27 2020, 10:05 AM
In T225977#6083284, @DannyS712 wrote:

Suggest declining per T251073: Hard deprecate Revision::getPrevious

Revision::getPrevious() presumably returns null because RevisionStore::getPreviousRevision() returns null. Changing the caller from the old to the new method would not resolve the problem.

daniel added a comment.Apr 27 2020, 10:06 AM

Changing the Core code to use READ_LATEST still does not resolve this issue, so I don't believe it is any sort of race condition.

Did you try that flag in the call to getPreviousRevision(), or just the Revision constructor? The former is the critical one, the later is unlikely to change anything.

daniel edited projects, added Platform Team Workboards (Clinic Duty Team); removed Platform Engineering.Apr 28 2020, 12:01 PM
daniel moved this task from Inbox to Later on the Platform Team Workboards (Clinic Duty Team) board.Apr 28 2020, 7:16 PM
daniel moved this task from Later to Discussing on the Platform Team Workboards (Clinic Duty Team) board.
• MusikAnimal added a comment.Apr 30 2020, 2:31 AM
In T225977#6084247, @daniel wrote:

Changing the Core code to use READ_LATEST still does not resolve this issue, so I don't believe it is any sort of race condition.

Did you try that flag in the call to getPreviousRevision(), or just the Revision constructor? The former is the critical one, the later is unlikely to change anything.

From T223828#5238155 I'm guessing I only did this in the constructor, so that might be the issue.

This discovery happened when Community Tech was working on PageTriage. That project has since concluded, so I don't have as much time to devote to this. I created this task just to document my findings. It may very well be invalid.

• eprodromou triaged this task as Low priority.Jun 2 2020, 8:43 PM
• eprodromou moved this task from Discussing to Later on the Platform Team Workboards (Clinic Duty Team) board.
• eprodromou subscribed.

We think this needs more examination, so we're going to take a closer look.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits