From: daniel Date: Tue, 19 Jun 2018 17:20:53 +0000 (+0200) Subject: Make undo fail if more than just the main slot is affected. X-Git-Tag: 1.34.0-rc.0~4993^2 X-Git-Url: http://git.heureux-cyclage.org/?a=commitdiff_plain;h=ef1edcea3c0315b013807181368c5c79ffdbadc2;p=lhc%2Fweb%2Fwiklou.git Make undo fail if more than just the main slot is affected. Bug: T194412 Change-Id: Ifdf9bc9d884844f9ffeb8019d9b13d5737862063 --- diff --git a/includes/EditPage.php b/includes/EditPage.php index 92097616f7..521ea5a562 100644 --- a/includes/EditPage.php +++ b/includes/EditPage.php @@ -1184,6 +1184,7 @@ class EditPage { if ( $undo > 0 && $undoafter > 0 ) { $undorev = Revision::newFromId( $undo ); $oldrev = Revision::newFromId( $undoafter ); + $undoMsg = null; # Sanity check, make sure it's the right page, # the revisions exist and they were not deleted. @@ -1192,12 +1193,19 @@ class EditPage { !$undorev->isDeleted( Revision::DELETED_TEXT ) && !$oldrev->isDeleted( Revision::DELETED_TEXT ) ) { - $content = $this->page->getUndoContent( $undorev, $oldrev ); - - if ( $content === false ) { - # Warn the user that something went wrong - $undoMsg = 'failure'; + if ( WikiPage::hasDifferencesOutsideMainSlot( $undorev, $oldrev ) ) { + // Cannot yet undo edits that involve anything other the main slot. + $undoMsg = 'main-slot-only'; } else { + $content = $this->page->getUndoContent( $undorev, $oldrev ); + + if ( $content === false ) { + # Warn the user that something went wrong + $undoMsg = 'failure'; + } + } + + if ( $undoMsg === null ) { $oldContent = $this->page->getContent( Revision::RAW ); $popts = ParserOptions::newFromUserAndLang( $user, $wgContLang ); $newContent = $content->preSaveTransform( $this->mTitle, $user, $popts ); @@ -1254,7 +1262,8 @@ class EditPage { } $out = $this->context->getOutput(); - // Messages: undo-success, undo-failure, undo-norev, undo-nochange + // Messages: undo-success, undo-failure, undo-main-slot-only, undo-norev, + // undo-nochange. $class = ( $undoMsg == 'success' ? '' : 'error ' ) . "mw-undo-{$undoMsg}"; $this->editFormPageTop .= $out->parse( "
" . $this->context->msg( 'undo-' . $undoMsg )->plain() . '
', true, /* interface */true ); diff --git a/includes/Storage/RevisionSlots.php b/includes/Storage/RevisionSlots.php index f37e722bba..ba9780f66b 100644 --- a/includes/Storage/RevisionSlots.php +++ b/includes/Storage/RevisionSlots.php @@ -270,4 +270,43 @@ class RevisionSlots { return true; } + /** + * Find roles for which the $other RevisionSlots object has different content + * as this RevisionSlots object, including any roles that are present in one + * but not the other. + * + * @param RevisionSlots $other + * + * @return string[] a list of slot roles that are different. + */ + public function getRolesWithDifferentContent( RevisionSlots $other ) { + if ( $other === $this ) { + return []; + } + + $aSlots = $this->getSlots(); + $bSlots = $other->getSlots(); + + ksort( $aSlots ); + ksort( $bSlots ); + + $different = array_keys( array_merge( + array_diff_key( $aSlots, $bSlots ), + array_diff_key( $bSlots, $aSlots ) + ) ); + + /** @var SlotRecord[] $common */ + $common = array_intersect_key( $aSlots, $bSlots ); + + foreach ( $common as $role => $s ) { + $t = $bSlots[$role]; + + if ( !$s->hasSameContent( $t ) ) { + $different[] = $role; + } + } + + return $different; + } + } diff --git a/includes/content/ContentHandler.php b/includes/content/ContentHandler.php index 3cfac8f90b..3b54494889 100644 --- a/includes/content/ContentHandler.php +++ b/includes/content/ContentHandler.php @@ -1069,7 +1069,7 @@ abstract class ContentHandler { * @param Revision $undo The revision to undo * @param Revision $undoafter Must be an earlier revision than $undo * - * @return mixed String on success, false on failure + * @return mixed Content on success, false on failure */ public function getUndoContent( Revision $current, Revision $undo, Revision $undoafter ) { $cur_content = $current->getContent(); diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index 5bbdb6cdec..75bee39231 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -1453,17 +1453,45 @@ class WikiPage implements Page, IDBAccessObject { return $ret; } + /** + * Helper method for checking whether two revisions have differences that go + * beyond the main slot. + * + * MCR migration note: this method should go away! + * + * @deprecated Use only as a stop-gap before refactoring to support MCR. + * + * @param Revision $a + * @param Revision $b + * @return bool + */ + public static function hasDifferencesOutsideMainSlot( Revision $a, Revision $b ) { + $aSlots = $a->getRevisionRecord()->getSlots(); + $bSlots = $b->getRevisionRecord()->getSlots(); + $changedRoles = $aSlots->getRolesWithDifferentContent( $bSlots ); + + return ( $changedRoles !== [ 'main' ] ); + } + /** * Get the content that needs to be saved in order to undo all revisions * between $undo and $undoafter. Revisions must belong to the same page, * must exist and must not be deleted + * * @param Revision $undo * @param Revision $undoafter Must be an earlier revision than $undo * @return Content|bool Content on success, false on failure * @since 1.21 * Before we had the Content object, this was done in getUndoText */ - public function getUndoContent( Revision $undo, Revision $undoafter = null ) { + public function getUndoContent( Revision $undo, Revision $undoafter ) { + // TODO: MCR: replace this with a method that returns a RevisionSlotsUpdate + + if ( self::hasDifferencesOutsideMainSlot( $undo, $undoafter ) ) { + // Cannot yet undo edits that involve anything other the main slot. + return false; + } + $handler = $undo->getContentHandler(); return $handler->getUndoContent( $this->getRevision(), $undo, $undoafter ); } diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 86a05f0418..9a7e8f1980 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -807,6 +807,7 @@ "converter-manual-rule-error": "Error detected in manual language conversion rule", "undo-success": "The edit can be undone.\nPlease check the comparison below to verify that this is what you want to do, and then save the changes below to finish undoing the edit.", "undo-failure": "The edit could not be undone due to conflicting intermediate edits.", + "undo-main-slot-only": "The edit could not be undone because it involves content outside the main slot.", "undo-norev": "The edit could not be undone because it does not exist or was deleted.", "undo-nochange": "The edit appears to have already been undone.", "undo-summary": "Undo revision $1 by [[Special:Contributions/$2|$2]] ([[User talk:$2|talk]])", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 0b920098ed..07a0959c43 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -1007,6 +1007,7 @@ "converter-manual-rule-error": "Used as error message when a manual conversion rule for the [[mw:Language_converter|language converter]] has errors. For example it's not using the correct syntax, or not supplying text in all variants.", "undo-success": "Text on special page to confirm edit revert. You arrive on this page by clicking on the \"undo\" link on a revision history special page.\n\n{{Identical|Undo}}", "undo-failure": "Message appears if an attempt to revert an edit by clicking the \"undo\" link on the page history fails.\n\nSee also:\n* {{msg-mw|Undo-norev}}\n* {{msg-mw|Undo-nochange}}\n{{Identical|Undo}}", + "undo-main-slot-only": "Message appears if an attempt to revert an edit by clicking the \"undo\" link on the page history fails because it involves content outside the page's main slot, which is not yet supported.\nThis message is temporary, see https://phabricator.wikimedia.org/T189808\n\nSee also:\n* {{msg-mw|Undo-failure}}\n{{Identical|Undo}}", "undo-norev": "Message appears if an attempt to revert an edit by clicking the \"undo\" link on the page history fails.\n\nSee also:\n* {{msg-mw|Undo-failure}}\n* {{msg-mw|Undo-nochange}}\n{{Identical|Undo}}", "undo-nochange": "Message appears if an attempt to revert an edit by clicking the \"undo\" link results in an edit making no change to the current version of the page.\n\nSee also:\n* {{msg-mw|Undo-failure}}\n* {{msg-mw|Undo-norev}}", "undo-summary": "Edit summary for an undo action. Parameters:\n* $1 - revision ID\n* $2 - username\n{{Identical|Undo}}", diff --git a/tests/phpunit/includes/Storage/RevisionSlotsTest.php b/tests/phpunit/includes/Storage/RevisionSlotsTest.php index ef14a9e1d2..52647c27bc 100644 --- a/tests/phpunit/includes/Storage/RevisionSlotsTest.php +++ b/tests/phpunit/includes/Storage/RevisionSlotsTest.php @@ -224,4 +224,34 @@ class RevisionSlotsTest extends MediaWikiTestCase { $this->assertSame( $same, $b->hasSameContent( $a ) ); } + public function provideGetRolesWithDifferentContent() { + $fooX = SlotRecord::newUnsaved( 'x', new TextContent( 'Foo' ) ); + $barZ = SlotRecord::newUnsaved( 'z', new TextContent( 'Bar' ) ); + $fooY = SlotRecord::newUnsaved( 'y', new TextContent( 'Foo' ) ); + $barZS = SlotRecord::newSaved( 7, 7, 'xyz', $barZ ); + $barZ2 = SlotRecord::newUnsaved( 'z', new TextContent( 'Baz' ) ); + + $a = $this->newRevisionSlots( [ 'x' => $fooX, 'z' => $barZ ] ); + $a2 = $this->newRevisionSlots( [ 'x' => $fooX, 'z' => $barZ ] ); + $a3 = $this->newRevisionSlots( [ 'x' => $fooX, 'z' => $barZS ] ); + $b = $this->newRevisionSlots( [ 'y' => $fooY, 'z' => $barZ ] ); + $c = $this->newRevisionSlots( [ 'x' => $fooX, 'z' => $barZ2 ] ); + + yield 'same instance' => [ $a, $a, [] ]; + yield 'same slots' => [ $a, $a2, [] ]; + yield 'same content' => [ $a, $a3, [] ]; + + yield 'different roles' => [ $a, $b, [ 'x', 'y' ] ]; + yield 'different content' => [ $a, $c, [ 'z' ] ]; + } + + /** + * @dataProvider provideGetRolesWithDifferentContent + * @covers \MediaWiki\Storage\RevisionSlots::getRolesWithDifferentContent + */ + public function testGetRolesWithDifferentContent( RevisionSlots $a, RevisionSlots $b, $roles ) { + $this->assertArrayEquals( $roles, $a->getRolesWithDifferentContent( $b ) ); + $this->assertArrayEquals( $roles, $b->getRolesWithDifferentContent( $a ) ); + } + }