EditResult's revert is not marked correctly when using undoafter param
Closed, ResolvedPublic
Actions

Description

As @Proc has pointed out to me on IRC, it seems EditResult's revert-related fields are not set correctly when using the undoafter URL parameter. Weirdly enough, if one tries to revert just one revision (so undoafter is set to be the second-to-last revision) it works fine. The problem only occurs when the undo spans multiple edits.

There's some strange magic going, I'll try to fix it ASAP.

Details

Subject	Repo	Branch	Lines +/-
EditPage: handle undoafter param properly	mediawiki/core	REL1_35	+487 -233
EditPage: handle undoafter param properly	mediawiki/core	master	+487 -233
Handle undos in action=mcrundo properly	mediawiki/core	master	+244 -2

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	SBisson	T162675 'Integrated filters'—add all RC Page filters into the new UI along with selected new functions
Resolved	MMiller_WMF	T164307 Add Reverted filter to RecentChanges Filters
Stalled	None	T159725 AbuseFilter: Indicate that an edit was a revert
Declined	None	T155512 Trending service should track reverts more accurately
Resolved	Ostrzyciel	T56007 "reverted"-type notifications should be based on the actual page content
Resolved	Ostrzyciel	T152434 Add method to Revision to check if it was a Revert, and whether an edit was Reverted
Resolved	Ostrzyciel	T248775 Proposal: Add Reverted filter to RecentChanges Filters
Resolved	Ostrzyciel	T254074 Implement the reverted edit tag
Resolved	Ostrzyciel	T258952 PageUpdater: implement EditResult class
Resolved	Ostrzyciel	T256915 EditResult's revert is not marked correctly when using undoafter param

Event Timeline

Ostrzyciel created this task.Jul 1 2020, 7:33 PM

Alright, I'm really confused now. Let's break things down:

There is a class named McrUndoAction that seems to contain all the ingredients necessary to properly mark an edit as a revert with the undoafter param. It is, however, deprecated since 1.32 and only accessible through calls to index.php?action=mcrundo which is just... wat.
- EditResult-related code in this class is a bit broken, but mostly functional. The undoafter param works, but the oldest reverted ID is set incorrectly. I will fix that later.
In case we call action=edit, for either index.php or api.php, the request is routed to EditPage and then WikiPage. WikiPage::doEditContent (which is deprecated BTW) receives the undid revision id through its $undidRevId param. That works fine only single edit undos.
In case we use action=edit with the undoafter param:
- When the undo is done through index.php, EditPage "loses" the undo-related parameters entirely and nothing gets passed on to WikiPage::doEditContent, not even the top undid revision, which results in the edit not being marked as a revert.
- If we do the same through api.php, EditPage passes on to WikiPage the top undid revision ID correctly, but not the undoafter param, as there is no way to do that. The edit is marked as a revert, though, which I guess is a partial success.

So…?

I can fix the McrUndoAction easily, but I have no clue what is the current grand plan regarding the EditPage class. Is it waiting for a large refactor? I just don't know where to start with it. EditPage is a tangled mess that scares me a bit.

Change 609227 had a related patch set uploaded (by Ostrzyciel; owner: Ostrzyciel):
[mediawiki/core@master] Handle undos in action=mcrundo properly

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /609227

gerritbot added a project: Patch-For-Review.Jul 2 2020, 7:51 PM

Reedy added a project: Platform Engineering.Jul 4 2020, 12:30 PM

Change 609517 had a related patch set uploaded (by Ostrzyciel; owner: Ostrzyciel):
[mediawiki/core@master] WIP: EditPage: handle undoafter param properly

https://gerrit.wikimedia.org/r/609517

• eprodromou edited projects, added Platform Team Workboards (Clinic Duty Team); removed Platform Engineering.Jul 7 2020, 8:23 PM

• eprodromou edited projects, added Platform Team Workboards (External Code Reviews); removed Platform Team Workboards (Clinic Duty Team).

Change 609227 merged by jenkins-bot:
[mediawiki/core@master] Handle undos in action=mcrundo properly

https://gerrit.wikimedia.org/r/609227

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.41; 2020-07-14).Jul 10 2020, 10:00 AM

Change 609517 merged by jenkins-bot:
[mediawiki/core@master] EditPage: handle undoafter param properly

https://gerrit.wikimedia.org/r/609517

Change 612493 had a related patch set uploaded (by Kosta Harlan; owner: Ostrzyciel):
[mediawiki/core@REL1_35] EditPage: handle undoafter param properly

https://gerrit.wikimedia.org/r/612493

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.1; 2020-07-21).Jul 14 2020, 2:00 PM

Just a status update, the two relevant patches were merged into master and one of them is waiting for a backport to 1.35.

All issues should be resolved on master, I'll close this when the backport is finished.

Change 612493 merged by jenkins-bot:
[mediawiki/core@REL1_35] EditPage: handle undoafter param properly

https://gerrit.wikimedia.org/r/612493

ReleaseTaggerBot edited projects, added MW-1.35-notes; removed MW-1.35-notes (1.35.0-wmf.41; 2020-07-14).Jul 22 2020, 10:00 PM

Ostrzyciel closed this task as Resolved.Jul 23 2020, 9:21 AM

Ostrzyciel edited parent tasks, added: T258952: PageUpdater: implement EditResult class; removed: T254074: Implement the reverted edit tag.Jul 27 2020, 2:28 PM

Ostrzyciel mentioned this in T248775: Proposal: Add Reverted filter to RecentChanges Filters.Aug 30 2020, 8:51 AM

EditResult's revert is not marked correctly when using undoafter paramClosed, ResolvedPublicActions