Move non-user specific things from Title::isValidMoveOperation() to MovePage

Change-Id: Ieffeb0c7a15b202dcbdaf2a9d0b9bcdc10e360d2

diff --git a/includes/MovePage.php b/includes/MovePage.php
index fdece8d..cea91d2 100644 (file)
--- a/includes/MovePage.php
+++ b/includes/MovePage.php
@@ -42,6 +42,90 @@ class MovePage {
                $this->newTitle = $newTitle;
        }
 
+       /**
+        * Does various sanity checks that the move is
+        * valid. Only things based on the two titles
+        * should be checked here.
+        *
+        * @return Status
+        */
+       public function isValidMove() {
+               global $wgContentHandlerUseDB;
+               $status = new Status();
+
+               if ( $this->oldTitle->equals( $this->newTitle ) ) {
+                       $status->fatal( 'selfmove' );
+               }
+               if ( !$this->oldTitle->isMovable() ) {
+                       $status->fatal( 'immobile-source-namespace', $this->oldTitle->getNsText() );
+               }
+               if ( $this->newTitle->isExternal() ) {
+                       $status->fatal( 'immobile-target-namespace-iw' );
+               }
+               if ( !$this->newTitle->isMovable() ) {
+                       $status->fatal( 'immobile-target-namespace', $this->newTitle->getNsText() );
+               }
+
+               $oldid = $this->oldTitle->getArticleID();
+
+               if ( strlen( $this->newTitle->getDBkey() ) < 1 ) {
+                       $errors[] = array( 'articleexists' );
+               }
+               if (
+                       ( $this->oldTitle->getDBkey() == '' ) ||
+                       ( !$oldid ) ||
+                       ( $this->newTitle->getDBkey() == '' )
+               ) {
+                       $errors[] = array( 'badarticleerror' );
+               }
+
+               // Content model checks
+               if ( !$wgContentHandlerUseDB &&
+                       $this->oldTitle->getContentModel() !== $this->newTitle->getContentModel() ) {
+                       // can't move a page if that would change the page's content model
+                       $status->fatal(
+                               'bad-target-model',
+                               ContentHandler::getLocalizedName( $this->oldTitle->getContentModel() ),
+                               ContentHandler::getLocalizedName( $this->newTitle->getContentModel() )
+                       );
+               }
+
+               // Image-specific checks
+               if ( $this->oldTitle->inNamespace( NS_FILE ) ) {
+                       $status->merge( $this->isValidFileMove() );
+               }
+
+               if ( $this->newTitle->inNamespace( NS_FILE ) && !$this->oldTitle->inNamespace( NS_FILE ) ) {
+                       $status->fatal( 'nonfile-cannot-move-to-file' );
+               }
+
+               return $status;
+       }
+
+       /**
+        * Sanity checks for when a file is being moved
+        *
+        * @return Status
+        */
+       protected function isValidFileMove() {
+               $status = new Status();
+               $file = wfLocalFile( $this->oldTitle );
+               if ( $file->exists() ) {
+                       if ( $this->newTitle->getText() != wfStripIllegalFilenameChars( $this->newTitle->getText() ) ) {
+                               $status->fatal( 'imageinvalidfilename' );
+                       }
+                       if ( !File::checkExtensionCompatibility( $file, $this->newTitle->getDBkey() ) ) {
+                               $status->fatal( 'imagetypemismatch' );
+                       }
+               }
+
+               if ( !$this->newTitle->inNamespace( NS_FILE ) ) {
+                       $status->fatal( 'imagenocrossnamespace' );
+               }
+
+               return $status;
+       }
+
        /**
         * @param User $user
         * @param string $reason

diff --git a/includes/Title.php b/includes/Title.php
index 74d78ba..e8cda85 100644 (file)
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -3589,7 +3589,7 @@ class Title {
         * Check whether a given move operation would be valid.
         * Returns true if ok, or a getUserPermissionsErrors()-like array otherwise
         *
-        * @todo move this into MovePage
+        * @todo finish moving this into MovePage
         * @param Title $nt The new title
         * @param bool $auth Indicates whether $wgUser's permissions
         *  should be checked
@@ -3597,60 +3597,18 @@ class Title {
         * @return array|bool True on success, getUserPermissionsErrors()-like array on failure
         */
        public function isValidMoveOperation( &$nt, $auth = true, $reason = '' ) {
-               global $wgUser, $wgContentHandlerUseDB;
+               global $wgUser;
 
-               $errors = array();
-               if ( !$nt ) {
+               if ( !( $nt instanceof Title ) ) {
                        // Normally we'd add this to $errors, but we'll get
                        // lots of syntax errors if $nt is not an object
                        return array( array( 'badtitletext' ) );
                }
-               if ( $this->equals( $nt ) ) {
-                       $errors[] = array( 'selfmove' );
-               }
-               if ( !$this->isMovable() ) {
-                       $errors[] = array( 'immobile-source-namespace', $this->getNsText() );
-               }
-               if ( $nt->isExternal() ) {
-                       $errors[] = array( 'immobile-target-namespace-iw' );
-               }
-               if ( !$nt->isMovable() ) {
-                       $errors[] = array( 'immobile-target-namespace', $nt->getNsText() );
-               }
-
-               $oldid = $this->getArticleID();
-               $newid = $nt->getArticleID();
-
-               if ( strlen( $nt->getDBkey() ) < 1 ) {
-                       $errors[] = array( 'articleexists' );
-               }
-               if (
-                       ( $this->getDBkey() == '' ) ||
-                       ( !$oldid ) ||
-                       ( $nt->getDBkey() == '' )
-               ) {
-                       $errors[] = array( 'badarticleerror' );
-               }
-
-               // Content model checks
-               if ( !$wgContentHandlerUseDB &&
-                               $this->getContentModel() !== $nt->getContentModel() ) {
-                       // can't move a page if that would change the page's content model
-                       $errors[] = array(
-                               'bad-target-model',
-                               ContentHandler::getLocalizedName( $this->getContentModel() ),
-                               ContentHandler::getLocalizedName( $nt->getContentModel() )
-                       );
-               }
 
-               // Image-specific checks
-               if ( $this->getNamespace() == NS_FILE ) {
-                       $errors = array_merge( $errors, $this->validateFileMoveOperation( $nt ) );
-               }
+               $mp = new MovePage( $this, $nt );
+               $errors = $mp->isValidMove()->getErrorsArray();
 
-               if ( $nt->getNamespace() == NS_FILE && $this->getNamespace() != NS_FILE ) {
-                       $errors[] = array( 'nonfile-cannot-move-to-file' );
-               }
+               $newid = $nt->getArticleID();
 
                if ( $auth ) {
                        $errors = wfMergeErrorArrays( $errors,
@@ -3700,6 +3658,7 @@ class Title {
 
        /**
         * Check if the requested move target is a valid file move target
+        * @todo move this to MovePage
         * @param Title $nt Target title
         * @return array List of errors
         */
@@ -3708,27 +3667,6 @@ class Title {
 
                $errors = array();
 
-               // wfFindFile( $nt ) / wfLocalFile( $nt ) is not allowed until below
-
-               $file = wfLocalFile( $this );
-               if ( $file->exists() ) {
-                       if ( $nt->getText() != wfStripIllegalFilenameChars( $nt->getText() ) ) {
-                               $errors[] = array( 'imageinvalidfilename' );
-                       }
-                       if ( !File::checkExtensionCompatibility( $file, $nt->getDBkey() ) ) {
-                               $errors[] = array( 'imagetypemismatch' );
-                       }
-               }
-
-               if ( $nt->getNamespace() != NS_FILE ) {
-                       $errors[] = array( 'imagenocrossnamespace' );
-                       // From here we want to do checks on a file object, so if we can't
-                       // create one, we must return.
-                       return $errors;
-               }
-
-               // wfFindFile( $nt ) / wfLocalFile( $nt ) is allowed below here
-
                $destFile = wfLocalFile( $nt );
                if ( !$wgUser->isAllowed( 'reupload-shared' ) && !$destFile->exists() && wfFindFile( $nt ) ) {
                        $errors[] = array( 'file-exists-sharedrepo' );
@@ -3899,6 +3837,7 @@ class Title {
         * Checks if $this can be moved to a given Title
         * - Selects for update, so don't call it unless you mean business
         *
+        * @todo move to MovePage
         * @param Title $nt The new title to check
         * @return bool
         */

diff --git a/tests/phpunit/includes/MovePageTest.php b/tests/phpunit/includes/MovePageTest.php
new file mode 100644 (file)
index 0000000..027b877
--- /dev/null
+++ b/tests/phpunit/includes/MovePageTest.php
@@ -0,0 +1,39 @@
+<?php
+
+class MovePageTest extends MediaWikiTestCase {
+
+       /**
+        * @dataProvider provideIsValidMove
+        * @covers MovePage::isValidMove
+        * @covers MovePage::isValidFileMove
+        */
+       public function testIsValidMove( $old, $new, $error ) {
+               $this->setMwGlobals( 'wgContentHandlerUseDB', false );
+               $mp = new MovePage(
+                       Title::newFromText( $old ),
+                       Title::newFromText( $new )
+               );
+               $status = $mp->isValidMove();
+               if ( $error === true ) {
+                       $this->assertTrue( $status->isGood() );
+               } else {
+                       $this->assertTrue( $status->hasMessage( $error ) );
+               }
+       }
+
+       /**
+        * This should be kept in sync with TitleTest::provideTestIsValidMoveOperation
+        */
+       public static function provideIsValidMove() {
+               return array(
+                       // for MovePage::isValidMove
+                       array( 'Test', 'Test', 'selfmove' ),
+                       array( 'Special:FooBar', 'Test', 'immobile-source-namespace' ),
+                       array( 'Test', 'Special:FooBar', 'immobile-target-namespace' ),
+                       array( 'MediaWiki:Common.js', 'Help:Some wikitext page', 'bad-target-model' ),
+                       array( 'Page', 'File:Test.jpg', 'nonfile-cannot-move-to-file' ),
+                       // for MovePage::isValidFileMove
+                       array( 'File:Test.jpg', 'Page', 'imagenocrossnamespace' ),
+               );
+       }
+}