Introduce MovePage::moveSubpages(IfAllowed)
authorAryeh Gregor <ayg@aryeh.name>
Tue, 16 Apr 2019 13:51:04 +0000 (16:51 +0300)
committerBill Pirkle <bpirkle@wikimedia.org>
Wed, 22 May 2019 19:44:27 +0000 (14:44 -0500)
Title::moveSubpages() is now deprecated, and the one caller in core and
extensions was ported.

Change-Id: Ic1dc5a6f1a1bef89a35c13f0a72f6740c6a01c50

RELEASE-NOTES-1.34
includes/MovePage.php
includes/Title.php
includes/api/ApiMove.php
tests/phpunit/includes/MovePageTest.php

index f1bdfd3..a62c548 100644 (file)
@@ -215,6 +215,8 @@ because of Phabricator reports.
   should be used when creating any temporary blocks.
 * Parser::$mConf is deprecated. It will be removed entirely in a later version.
 * Constructing Parser directly is deprecated. Obtain one from ParserFactory.
+* Title::moveSubpages is deprecated. Use MovePage::moveSubpages or
+  MovePage::moveSubpagesIfAllowed.
 
 === Other changes in 1.34 ===
 * …
index 004ca07..d045355 100644 (file)
@@ -286,6 +286,132 @@ class MovePage {
                return $this->moveUnsafe( $user, $reason, $createRedirect, $changeTags );
        }
 
+       /**
+        * Move the source page's subpages to be subpages of the target page, without checking user
+        * permissions. The caller is responsible for moving the source page itself. We will still not
+        * do moves that are inherently not allowed, nor will we move more than $wgMaximumMovedPages.
+        *
+        * @param User $user
+        * @param string|null $reason The reason for the move
+        * @param bool|null $createRedirect Whether to create redirects from the old subpages to
+        *  the new ones
+        * @param string[] $changeTags Applied to entries in the move log and redirect page revision
+        * @return Status Good if no errors occurred. Ok if at least one page succeeded. The "value"
+        *  of the top-level status is an array containing the per-title status for each page. For any
+        *  move that succeeded, the "value" of the per-title status is the new page title.
+        */
+       public function moveSubpages(
+               User $user, $reason = null, $createRedirect = true, array $changeTags = []
+       ) {
+               return $this->moveSubpagesInternal( false, $user, $reason, $createRedirect, $changeTags );
+       }
+
+       /**
+        * Move the source page's subpages to be subpages of the target page, with user permission
+        * checks. The caller is responsible for moving the source page itself.
+        *
+        * @param User $user
+        * @param string|null $reason The reason for the move
+        * @param bool|null $createRedirect Whether to create redirects from the old subpages to
+        *  the new ones. Ignored if the user doesn't have the 'suppressredirect' right.
+        * @param string[] $changeTags Applied to entries in the move log and redirect page revision
+        * @return Status Good if no errors occurred. Ok if at least one page succeeded. The "value"
+        *  of the top-level status is an array containing the per-title status for each page. For any
+        *  move that succeeded, the "value" of the per-title status is the new page title.
+        */
+       public function moveSubpagesIfAllowed(
+               User $user, $reason = null, $createRedirect = true, array $changeTags = []
+       ) {
+               return $this->moveSubpagesInternal( true, $user, $reason, $createRedirect, $changeTags );
+       }
+
+       /**
+        * @param bool $checkPermissions
+        * @param User $user
+        * @param string $reason
+        * @param bool $createRedirect
+        * @param array $changeTags
+        * @return Status
+        */
+       private function moveSubpagesInternal(
+               $checkPermissions, User $user, $reason, $createRedirect, array $changeTags
+       ) {
+               global $wgMaximumMovedPages;
+               $services = MediaWikiServices::getInstance();
+
+               if ( $checkPermissions ) {
+                       if ( !$services->getPermissionManager()->userCan(
+                               'move-subpages', $user, $this->oldTitle )
+                       ) {
+                               return Status::newFatal( 'cant-move-subpages' );
+                       }
+               }
+
+               $nsInfo = $services->getNamespaceInfo();
+
+               // Do the source and target namespaces support subpages?
+               if ( !$nsInfo->hasSubpages( $this->oldTitle->getNamespace() ) ) {
+                       return Status::newFatal( 'namespace-nosubpages',
+                               $nsInfo->getCanonicalName( $this->oldTitle->getNamespace() ) );
+               }
+               if ( !$nsInfo->hasSubpages( $this->newTitle->getNamespace() ) ) {
+                       return Status::newFatal( 'namespace-nosubpages',
+                               $nsInfo->getCanonicalName( $this->newTitle->getNamespace() ) );
+               }
+
+               // Return a status for the overall result. Its value will be an array with per-title
+               // status for each subpage. Merge any errors from the per-title statuses into the
+               // top-level status without resetting the overall result.
+               $topStatus = Status::newGood();
+               $perTitleStatus = [];
+               $subpages = $this->oldTitle->getSubpages( $wgMaximumMovedPages + 1 );
+               $count = 0;
+               foreach ( $subpages as $oldSubpage ) {
+                       $count++;
+                       if ( $count > $wgMaximumMovedPages ) {
+                               $status = Status::newFatal( 'movepage-max-pages', $wgMaximumMovedPages );
+                               $perTitleStatus[$oldSubpage->getPrefixedText()] = $status;
+                               $topStatus->merge( $status );
+                               $topStatus->setOk( true );
+                               break;
+                       }
+
+                       // We don't know whether this function was called before or after moving the root page,
+                       // so check both titles
+                       if ( $oldSubpage->getArticleID() == $this->oldTitle->getArticleID() ||
+                               $oldSubpage->getArticleID() == $this->newTitle->getArticleID()
+                       ) {
+                               // When moving a page to a subpage of itself, don't move it twice
+                               continue;
+                       }
+                       $newPageName = preg_replace(
+                                       '#^' . preg_quote( $this->oldTitle->getDBkey(), '#' ) . '#',
+                                       StringUtils::escapeRegexReplacement( $this->newTitle->getDBkey() ), # T23234
+                                       $oldSubpage->getDBkey() );
+                       if ( $oldSubpage->isTalkPage() ) {
+                               $newNs = $this->newTitle->getTalkPage()->getNamespace();
+                       } else {
+                               $newNs = $this->newTitle->getSubjectPage()->getNamespace();
+                       }
+                       // T16385: we need makeTitleSafe because the new page names may be longer than 255
+                       // characters.
+                       $newSubpage = Title::makeTitleSafe( $newNs, $newPageName );
+
+                       $mp = new MovePage( $oldSubpage, $newSubpage );
+                       $method = $checkPermissions ? 'moveIfAllowed' : 'move';
+                       $status = $mp->$method( $user, $reason, $createRedirect, $changeTags );
+                       if ( $status->isOK() ) {
+                               $status->setResult( true, $newSubpage->getPrefixedText() );
+                       }
+                       $perTitleStatus[$oldSubpage->getPrefixedText()] = $status;
+                       $topStatus->merge( $status );
+                       $topStatus->setOk( true );
+               }
+
+               $topStatus->value = $perTitleStatus;
+               return $topStatus;
+       }
+
        /**
         * Moves *without* any sort of safety or sanity checks. Hooks can still fail the move, however.
         *
index 866f041..c4fe858 100644 (file)
@@ -3480,6 +3480,7 @@ class Title implements LinkTarget, IDBAccessObject {
        /**
         * Move this page's subpages to be subpages of $nt
         *
+        * @deprecated since 1.34, use MovePage instead
         * @param Title $nt Move target
         * @param bool $auth Whether $wgUser's permissions should be checked
         * @param string $reason The reason for the move
@@ -3494,7 +3495,6 @@ class Title implements LinkTarget, IDBAccessObject {
        public function moveSubpages( $nt, $auth = true, $reason = '', $createRedirect = true,
                array $changeTags = []
        ) {
-               global $wgMaximumMovedPages;
                // Check permissions
                if ( !$this->userCan( 'move-subpages' ) ) {
                        return [
@@ -3514,46 +3514,21 @@ class Title implements LinkTarget, IDBAccessObject {
                        ];
                }
 
-               $subpages = $this->getSubpages( $wgMaximumMovedPages + 1 );
-               $retval = [];
-               $count = 0;
-               foreach ( $subpages as $oldSubpage ) {
-                       $count++;
-                       if ( $count > $wgMaximumMovedPages ) {
-                               $retval[$oldSubpage->getPrefixedText()] = [
-                                       [ 'movepage-max-pages', $wgMaximumMovedPages ],
-                               ];
-                               break;
-                       }
+               global $wgUser;
+               $mp = new MovePage( $this, $nt );
+               $method = $auth ? 'moveSubpagesIfAllowed' : 'moveSubpages';
+               $result = $mp->$method( $wgUser, $reason, $createRedirect, $changeTags );
 
-                       // We don't know whether this function was called before
-                       // or after moving the root page, so check both
-                       // $this and $nt
-                       if ( $oldSubpage->getArticleID() == $this->getArticleID()
-                               || $oldSubpage->getArticleID() == $nt->getArticleID()
-                       ) {
-                               // When moving a page to a subpage of itself,
-                               // don't move it twice
-                               continue;
-                       }
-                       $newPageName = preg_replace(
-                                       '#^' . preg_quote( $this->mDbkeyform, '#' ) . '#',
-                                       StringUtils::escapeRegexReplacement( $nt->getDBkey() ), # T23234
-                                       $oldSubpage->getDBkey() );
-                       if ( $oldSubpage->isTalkPage() ) {
-                               $newNs = $nt->getTalkPage()->getNamespace();
-                       } else {
-                               $newNs = $nt->getSubjectPage()->getNamespace();
-                       }
-                       # T16385: we need makeTitleSafe because the new page names may
-                       # be longer than 255 characters.
-                       $newSubpage = self::makeTitleSafe( $newNs, $newPageName );
+               if ( !$result->isOk() ) {
+                       return $result->getErrorsArray();
+               }
 
-                       $success = $oldSubpage->moveTo( $newSubpage, $auth, $reason, $createRedirect, $changeTags );
-                       if ( $success === true ) {
-                               $retval[$oldSubpage->getPrefixedText()] = $newSubpage->getPrefixedText();
+               $retval = [];
+               foreach ( $result->getValue() as $key => $status ) {
+                       if ( $status->isOK() ) {
+                               $retval[$key] = $status->getValue();
                        } else {
-                               $retval[$oldSubpage->getPrefixedText()] = $success;
+                               $retval[$key] = $status->getErrorsArray();
                        }
                }
                return $retval;
index cc4490e..89ecc43 100644 (file)
@@ -201,22 +201,22 @@ class ApiMove extends ApiBase {
        public function moveSubpages( $fromTitle, $toTitle, $reason, $noredirect, $changeTags = [] ) {
                $retval = [];
 
-               $success = $fromTitle->moveSubpages( $toTitle, true, $reason, !$noredirect, $changeTags );
-               if ( isset( $success[0] ) ) {
-                       $status = $this->errorArrayToStatus( $success );
-                       return [ 'errors' => $this->getErrorFormatter()->arrayFromStatus( $status ) ];
+               $mp = new MovePage( $fromTitle, $toTitle );
+               $result =
+                       $mp->moveSubpagesIfAllowed( $this->getUser(), $reason, !$noredirect, $changeTags );
+               if ( !$result->isOk() ) {
+                       // This means the whole thing failed
+                       return [ 'errors' => $this->getErrorFormatter()->arrayFromStatus( $result ) ];
                }
 
                // At least some pages could be moved
                // Report each of them separately
-               foreach ( $success as $oldTitle => $newTitle ) {
+               foreach ( $result->getValue() as $oldTitle => $status ) {
                        $r = [ 'from' => $oldTitle ];
-                       if ( is_array( $newTitle ) ) {
-                               $status = $this->errorArrayToStatus( $newTitle );
-                               $r['errors'] = $this->getErrorFormatter()->arrayFromStatus( $status );
+                       if ( $status->isOK() ) {
+                               $r['to'] = $status->getValue();
                        } else {
-                               // Success
-                               $r['to'] = $newTitle;
+                               $r['errors'] = $this->getErrorFormatter()->arrayFromStatus( $status );
                        }
                        $retval[] = $r;
                }
index 1b2b159..db9d2ab 100644 (file)
@@ -5,6 +5,13 @@
  */
 class MovePageTest extends MediaWikiTestCase {
 
+       public function setUp() {
+               parent::setUp();
+               $this->tablesUsed[] = 'page';
+               $this->tablesUsed[] = 'revision';
+               $this->tablesUsed[] = 'comment';
+       }
+
        /**
         * @dataProvider provideIsValidMove
         * @covers MovePage::isValidMove
@@ -83,4 +90,108 @@ class MovePageTest extends MediaWikiTestCase {
                $status = $mp->move( $user, 'Reason', true );
                $this->assertTrue( $status->hasMessage( $error ) );
        }
+
+       /**
+        * Test moving subpages from one page to another
+        * @covers MovePage::moveSubpages
+        */
+       public function testMoveSubpages() {
+               $name = ucfirst( __FUNCTION__ );
+
+               $subPages = [ "Talk:$name/1", "Talk:$name/2" ];
+               $ids = [];
+               $pages = [
+                       $name,
+                       "Talk:$name",
+                       "$name 2",
+                       "Talk:$name 2",
+               ];
+               foreach ( array_merge( $pages, $subPages ) as $page ) {
+                       $ids[$page] = $this->createPage( $page );
+               }
+
+               $oldTitle = Title::newFromText( "Talk:$name" );
+               $newTitle = Title::newFromText( "Talk:$name 2" );
+               $mp = new MovePage( $oldTitle, $newTitle );
+               $status = $mp->moveSubpages( $this->getTestUser()->getUser(), 'Reason', true );
+
+               $this->assertTrue( $status->isGood(),
+                       "Moving subpages from Talk:{$name} to Talk:{$name} 2 was not completely successful." );
+               foreach ( $subPages as $page ) {
+                       $this->assertMoved( $page, str_replace( $name, "$name 2", $page ), $ids[$page] );
+               }
+       }
+
+       /**
+        * Test moving subpages from one page to another
+        * @covers MovePage::moveSubpagesIfAllowed
+        */
+       public function testMoveSubpagesIfAllowed() {
+               $name = ucfirst( __FUNCTION__ );
+
+               $subPages = [ "Talk:$name/1", "Talk:$name/2" ];
+               $ids = [];
+               $pages = [
+                       $name,
+                       "Talk:$name",
+                       "$name 2",
+                       "Talk:$name 2",
+               ];
+               foreach ( array_merge( $pages, $subPages ) as $page ) {
+                       $ids[$page] = $this->createPage( $page );
+               }
+
+               $oldTitle = Title::newFromText( "Talk:$name" );
+               $newTitle = Title::newFromText( "Talk:$name 2" );
+               $mp = new MovePage( $oldTitle, $newTitle );
+               $status = $mp->moveSubpagesIfAllowed( $this->getTestUser()->getUser(), 'Reason', true );
+
+               $this->assertTrue( $status->isGood(),
+                       "Moving subpages from Talk:{$name} to Talk:{$name} 2 was not completely successful." );
+               foreach ( $subPages as $page ) {
+                       $this->assertMoved( $page, str_replace( $name, "$name 2", $page ), $ids[$page] );
+               }
+       }
+
+       /**
+        * Shortcut function to create a page and return its id.
+        *
+        * @param string $name Page to create
+        * @return int ID of created page
+        */
+       protected function createPage( $name ) {
+               return $this->editPage( $name, 'Content' )->value['revision']->getPage();
+       }
+
+       /**
+        * @param string $from Prefixed name of source
+        * @param string $to Prefixed name of destination
+        * @param string $id Page id of the page to move
+        * @param array|string|null $opts Options: 'noredirect' to expect no redirect
+        */
+       protected function assertMoved( $from, $to, $id, $opts = null ) {
+               $opts = (array)$opts;
+
+               Title::clearCaches();
+               $fromTitle = Title::newFromText( $from );
+               $toTitle = Title::newFromText( $to );
+
+               $this->assertTrue( $toTitle->exists(),
+                       "Destination {$toTitle->getPrefixedText()} does not exist" );
+
+               if ( in_array( 'noredirect', $opts ) ) {
+                       $this->assertFalse( $fromTitle->exists(),
+                               "Source {$fromTitle->getPrefixedText()} exists" );
+               } else {
+                       $this->assertTrue( $fromTitle->exists(),
+                               "Source {$fromTitle->getPrefixedText()} does not exist" );
+                       $this->assertTrue( $fromTitle->isRedirect(),
+                               "Source {$fromTitle->getPrefixedText()} is not a redirect" );
+
+                       $target = Revision::newFromTitle( $fromTitle )->getContent()->getRedirectTarget();
+                       $this->assertSame( $toTitle->getPrefixedText(), $target->getPrefixedText() );
+               }
+
+               $this->assertSame( $id, $toTitle->getArticleID() );
+       }
 }