Page MenuHomePhabricator

ApiEditPage.php assumes that higher revision numbers have later timestamps
Closed, ResolvedPublic


if ( $params['undo'] < $params['undoafter'] ) {
	list( $params['undo'], $params['undoafter'] ) =
		[ $params['undoafter'], $params['undo'] ];

This snippet helpfully checks if you reversed the order of the "undo" and "undoafter" parameters, and switches them if so. Unfortunately, it assumes that a higher revision ID indicates a later revision, which is not the case if old revisions are imported. In this case, it gets the undo backwards. Probably this special case could be removed entirely, assuming nobody's code depends on it, in which case if you reverse undo and undoafter you'll probably just get a merge conflict, right? Alternatively, we could add a check of the actual revision timestamps, but I didn't think about if that would be unduly inefficient.

Thanks to Anomie for pointing this out during code review of tests I was writing:

Event Timeline

Anomie added a project: good first task.
Anomie subscribed.

I did some queries on the Analytics data since Feb 1. There were only 14 requests to action=edit with undo < undoafter in that time. All are by the same user, and I was able to track down the script in question and let the maintainer know about this task.

I think we're safe to fix the bug.

The code in question is wrapped in a another block

                if ( $params['undoafter'] > 0 ) {
	          if ( $params['undo'] < $params['undoafter'] ) {
		    list( $params['undo'], $params['undoafter'] ) =
		    [ $params['undoafter'], $params['undo'] ];
		  $undoafterRev = Revision::newFromId( $params['undoafter'] );

It seems all that code does is create this problem. I'll submit a patch to get rid of it now.

Change 429533 had a related patch set uploaded (by DovAlp; owner: DovAlp):
[mediawiki/core@master] Fixes T190285

Simetrical, did the tests ever get changed. The code is edited but now the test suite is failing. The new tests should probably get merged in first.

Yes, the tests were merged. Your change will have to update the test to reflect the fixed behavior.

Note that includes/EditPage.php has the same special case and should presumably be fixed as well.

I can take a look at includes/EditPage.php later, but the original problem/tests have been fixed by my latest patch set. Let me know what you think.

(sorry for abandoning my patch for a while, life got in the way)

Change 429533 merged by jenkins-bot:
[mediawiki/core@master] ApiEditPage: Don't swap undo and undoafter parameters

Ammarpad assigned this task to DovAlp.
Ammarpad removed a project: Platform Engineering.