From 9510dbc4cee09ec0cec97325d99e44a5fc32b8f8 Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Tue, 16 Apr 2019 16:51:04 +0300 Subject: [PATCH] Introduce MovePage::moveSubpages(IfAllowed) Title::moveSubpages() is now deprecated, and the one caller in core and extensions was ported. Change-Id: Ic1dc5a6f1a1bef89a35c13f0a72f6740c6a01c50 --- RELEASE-NOTES-1.34 | 2 + includes/MovePage.php | 126 ++++++++++++++++++++++++ includes/Title.php | 51 +++------- includes/api/ApiMove.php | 20 ++-- tests/phpunit/includes/MovePageTest.php | 111 +++++++++++++++++++++ 5 files changed, 262 insertions(+), 48 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index f1bdfd30b1..a62c548349 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -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 === * … diff --git a/includes/MovePage.php b/includes/MovePage.php index 004ca07641..d04535558d 100644 --- a/includes/MovePage.php +++ b/includes/MovePage.php @@ -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. * diff --git a/includes/Title.php b/includes/Title.php index 866f0415d8..c4fe858ac8 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -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; diff --git a/includes/api/ApiMove.php b/includes/api/ApiMove.php index cc4490e63f..89ecc43bd9 100644 --- a/includes/api/ApiMove.php +++ b/includes/api/ApiMove.php @@ -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; } diff --git a/tests/phpunit/includes/MovePageTest.php b/tests/phpunit/includes/MovePageTest.php index 1b2b159f0a..db9d2ab8bc 100644 --- a/tests/phpunit/includes/MovePageTest.php +++ b/tests/phpunit/includes/MovePageTest.php @@ -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() ); + } } -- 2.20.1