Page MenuHomePhabricator

Investigate: Undoing and restore edit on a form does not work
Closed, ResolvedPublic

Description

Goal: Find out why it does not work. Brief other developers on the issue, so the estimation is possible.

Duration: 2h

Event Timeline

WMDE-leszek triaged this task as Normal priority.Mar 13 2018, 11:50 AM
WMDE-leszek created this task.
Addshore claimed this task.Mar 14 2018, 1:41 PM
Addshore moved this task from To Do to Doing on the Wikidata-Turtles-18.03.14 board.
Restricted Application added a project: User-Addshore. · View Herald TranscriptMar 14 2018, 1:41 PM

So debugging has lead me to SubmitEntityAction::undo in wikibase, and the following code snippet.

		if ( $patchedContent->equals( $latestContent ) ) {
			$status = Status::newGood();
			$status->warning( 'wikibase-empty-undo' );
		} else {
/// ...
                }

$patchedContent->equals( $latestContent ) returns true, so it appears that the undo action will do nothing hence nothing is done.
The 2 content objects here do indeed have the same content inside (both with the OLD data)

$latestRevision correctly has the latest revision ID and text.
$latestRevision->mSlots->slots->main->content{LexemeContent} has the correct blob of JSON, but the Lexeme entity within the content is incorrect showing the previous revision (not the latest)

I suspected this was due to LexemeHandler and the special handling for Forms in LexemeHandler::newEntityContent, as an EntityHolder can contain a specific revision of an Entity, and the special case for Forms will load an entity from an EntityLookup which will not have a specified version / revision. But it doesn't look like this special case code for forms it actually called during the undo submit.

It seems that the DeferredDecodingEntityHolder has one blob, the entity object it has does not match.
The trace of the first moment this occurs when DeferredDecodingEntityHolder::getEntity is called can be seen below... (something to do with redirects, doesn't seem relevant, but still probably an issue because of whatever underlying issue is causing all of this)

DeferredDecodingEntityHolder.php:98, Wikibase\Content\DeferredDecodingEntityHolder->getEntity()
DeferredDecodingEntityHolder.php:132, Wikibase\Content\DeferredDecodingEntityHolder->getEntityId()
DeferredCopyEntityHolder.php:60, Wikibase\Content\DeferredCopyEntityHolder->getEntityId()
EntityContent.php:129, Wikibase\Lexeme\Content\LexemeContent->getEntityId()
EntityContent.php:433, Wikibase\Lexeme\Content\LexemeContent->getRedirectData()
EntityContent.php:612, Wikibase\Lexeme\Content\LexemeContent->getPatchedRedirect()
EntityContent.php:586, Wikibase\Lexeme\Content\LexemeContent->getPatchedCopy()
SubmitEntityAction.php:160, Wikibase\SubmitEntityAction->getPatchContent()
SubmitEntityAction.php:113, Wikibase\SubmitEntityAction->undo()
SubmitEntityAction.php:66, Wikibase\SubmitEntityAction->show()
MediaWiki.php:500, MediaWiki->performAction()

The second trace looks a bit more like it might be the cause...

DeferredDecodingEntityHolder.php:98, Wikibase\Content\DeferredDecodingEntityHolder->getEntity()
DeferredCopyEntityHolder.php:47, Wikibase\Content\DeferredCopyEntityHolder->getEntity()
EntityContent.php:523, Wikibase\Lexeme\Content\LexemeContent->equals()
SubmitEntityAction.php:116, Wikibase\SubmitEntityAction->undo()
SubmitEntityAction.php:66, Wikibase\SubmitEntityAction->show()
MediaWiki.php:500, MediaWiki->performAction()

So how does the entity property of $latestRevision get changed to have the content that we want to undo to rather than actually having the latest content?

It looks like there some cloning logic missing from the Lexeme data model, so some mutable objects such as Form don't get cloned, so maintain their references to the originals.

Change 419450 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/WikibaseLexeme@master] WIP DNM Add __clone and copy to FormSet

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

It may also be worth doing a general audit of all other Lexeme DM objects to make sure no other clones that are needed are missing so we don't run into the same issue in other places / with other types of edit.

I can confirm that using the patch linked above, it seems that having cloning of FormSet added, undoing and restoring works fine.
The explanation provided above also seems legit to me.

Do you want to add a test "showcasing" the bug @Addshore, so we could have it fixed (with the patch you provided I assume), and shall we call the investigation done then, or do we call it done now?

Do you want to add a test "showcasing" the bug @Addshore, so we could have it fixed (with the patch you provided I assume), and shall we call the investigation done then, or do we call it done now?

I think we can leave the investigation now (and mark it as done) and when we come to tackle the actual task, write the test etc and actually merge the fix.

OK. The investigation is then done!

Change 419450 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] Add __clone and copy to FormSet

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

Jakob_WMDE closed this task as Resolved.Apr 3 2018, 10:20 AM

Change 423867 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/WikibaseLexeme@master] WIP SCRATCH: Investigate grammatical feature undo failings

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

Change 423867 abandoned by Addshore:
WIP SCRATCH: Investigate grammatical feature undo failings

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