Make undo fail if more than just the main slot is affected.
authordaniel <daniel.kinzler@wikimedia.de>
Tue, 19 Jun 2018 17:20:53 +0000 (19:20 +0200)
committerdaniel <daniel.kinzler@wikimedia.de>
Fri, 22 Jun 2018 13:18:41 +0000 (15:18 +0200)
Bug: T194412
Change-Id: Ifdf9bc9d884844f9ffeb8019d9b13d5737862063

includes/EditPage.php
includes/Storage/RevisionSlots.php
includes/content/ContentHandler.php
includes/page/WikiPage.php
languages/i18n/en.json
languages/i18n/qqq.json
tests/phpunit/includes/Storage/RevisionSlotsTest.php

index 9209761..521ea5a 100644 (file)
@@ -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( "<div class=\"{$class}\">" .
                                                $this->context->msg( 'undo-' . $undoMsg )->plain() . '</div>', true, /* interface */true );
index f37e722..ba9780f 100644 (file)
@@ -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;
+       }
+
 }
index 3cfac8f..3b54494 100644 (file)
@@ -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();
index 5bbdb6c..75bee39 100644 (file)
@@ -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 );
        }
index 86a05f0..9a7e8f1 100644 (file)
        "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]])",
index 0b92009..07a0959 100644 (file)
        "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}}",
index ef14a9e..52647c2 100644 (file)
@@ -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 ) );
+       }
+
 }