Return better errors from MovePage::isValidMove()
authorAryeh Gregor <ayg@aryeh.name>
Mon, 6 May 2019 11:14:24 +0000 (14:14 +0300)
committerAryeh Gregor <ayg@aryeh.name>
Mon, 19 Aug 2019 17:26:02 +0000 (20:26 +0300)
Previously the errors returned were incorrect or redundant in a number
of cases.

Change-Id: Ief96e69b0ae09afb9642f9ed93b2419a36351292

includes/MovePage.php
languages/i18n/en.json
languages/i18n/qqq.json
tests/phpunit/includes/MovePageTest.php

index 578c96d..a63eeae 100644 (file)
@@ -164,35 +164,33 @@ class MovePage {
 
                if ( $this->oldTitle->equals( $this->newTitle ) ) {
                        $status->fatal( 'selfmove' );
+               } elseif ( $this->newTitle->getArticleID() && !$this->isValidMoveTarget() ) {
+                       // The move is allowed only if (1) the target doesn't exist, or (2) the target is a
+                       // redirect to the source, and has no history (so we can undo bad moves right after
+                       // they're done).
+                       $status->fatal( 'articleexists' );
                }
-               if ( !$this->oldTitle->isMovable() ) {
+
+               // @todo If the old title is invalid, maybe we should check if it somehow exists in the
+               // database and allow moving it to a valid name? Why prohibit the move from an empty name
+               // without checking in the database?
+               if ( $this->oldTitle->getDBkey() == '' ) {
+                       $status->fatal( 'badarticleerror' );
+               } elseif ( $this->oldTitle->isExternal() ) {
+                       $status->fatal( 'immobile-source-namespace-iw' );
+               } elseif ( !$this->oldTitle->isMovable() ) {
                        $status->fatal( 'immobile-source-namespace', $this->oldTitle->getNsText() );
+               } elseif ( !$this->oldTitle->exists() ) {
+                       $status->fatal( 'movepage-source-doesnt-exist' );
                }
+
                if ( $this->newTitle->isExternal() ) {
                        $status->fatal( 'immobile-target-namespace-iw' );
-               }
-               if ( !$this->newTitle->isMovable() ) {
+               } elseif ( !$this->newTitle->isMovable() ) {
                        $status->fatal( 'immobile-target-namespace', $this->newTitle->getNsText() );
                }
-
-               $oldid = $this->oldTitle->getArticleID();
-
-               if ( $this->newTitle->getDBkey() === '' ) {
-                       $status->fatal( 'articleexists' );
-               }
-               if (
-                       ( $this->oldTitle->getDBkey() == '' ) ||
-                       ( !$oldid ) ||
-                       ( $this->newTitle->getDBkey() == '' )
-               ) {
-                       $status->fatal( 'badarticleerror' );
-               }
-
-               # The move is allowed only if (1) the target doesn't exist, or
-               # (2) the target is a redirect to the source, and has no history
-               # (so we can undo bad moves right after they're done).
-               if ( $this->newTitle->getArticleID() && !$this->isValidMoveTarget() ) {
-                       $status->fatal( 'articleexists' );
+               if ( !$this->newTitle->isValid() ) {
+                       $status->fatal( 'movepage-invalid-target-title' );
                }
 
                // Content model checks
@@ -237,6 +235,13 @@ class MovePage {
         */
        protected function isValidFileMove() {
                $status = new Status();
+
+               if ( !$this->newTitle->inNamespace( NS_FILE ) ) {
+                       $status->fatal( 'imagenocrossnamespace' );
+                       // No need for further errors about the target filename being wrong
+                       return $status;
+               }
+
                $file = $this->repoGroup->getLocalRepo()->newFile( $this->oldTitle );
                $file->load( File::READ_LATEST );
                if ( $file->exists() ) {
@@ -248,10 +253,6 @@ class MovePage {
                        }
                }
 
-               if ( !$this->newTitle->inNamespace( NS_FILE ) ) {
-                       $status->fatal( 'imagenocrossnamespace' );
-               }
-
                return $status;
        }
 
index 7bcda5b..56d0842 100644 (file)
        "move-subpages": "Move subpages (up to $1)",
        "move-talk-subpages": "Move subpages of talk page (up to $1)",
        "movepage-page-exists": "The page $1 already exists and cannot be automatically overwritten.",
+       "movepage-source-doesnt-exist": "The page $1 doesn't exist and cannot be moved.",
        "movepage-page-moved": "The page $1 has been moved to $2.",
        "movepage-page-unmoved": "The page $1 could not be moved to $2.",
        "movepage-max-pages": "The maximum of $1 {{PLURAL:$1|page|pages}} has been moved and no more will be moved automatically.",
        "delete_and_move_reason": "Deleted to make way for move from \"[[$1]]\"",
        "selfmove": "The title is the same;\ncannot move a page over itself.",
        "immobile-source-namespace": "Cannot move pages in namespace \"$1\".",
+       "immobile-source-namespace-iw": "Pages on other wikis cannot be moved from this wiki.",
        "immobile-target-namespace": "Cannot move pages into namespace \"$1\".",
        "immobile-target-namespace-iw": "Interwiki link is not a valid target for page move.",
        "immobile-source-page": "This page is not movable.",
        "immobile-target-page": "Cannot move to that destination title.",
+       "movepage-invalid-target-title": "The requested name is invalid.",
        "bad-target-model": "The desired destination uses a different content model. Cannot convert from $1 to $2.",
        "imagenocrossnamespace": "Cannot move file to non-file namespace.",
        "nonfile-cannot-move-to-file": "Cannot move non-file to file namespace.",
index a2ee29a..ae6379b 100644 (file)
        "move-subpages": "The text of an option on the special page [[Special:MovePage|MovePage]]. If this option is ticked, any subpages will be moved with the main page to a new title.\n\nParameters:\n* $1 - ...\nSee also:\n* {{msg-mw|Move-page-legend|legend for the form}}\n* {{msg-mw|newtitle|label for new title}}\n* {{msg-mw|Movereason|label for textarea}}\n* {{msg-mw|Movetalk|label for checkbox}}\n* {{msg-mw|Move-leave-redirect|label for checkbox}}\n* {{msg-mw|Fix-double-redirects|label for checkbox}}\n* {{msg-mw|Move-talk-subpages|label for checkbox}}\n* {{msg-mw|Move-watch|label for checkbox}}",
        "move-talk-subpages": "The text of an option on the special page [[Special:MovePage|MovePage]]. If this option is ticked, any talk subpages will be moved with the talk page to a new title.\n\nParameters:\n* $1 - ...\nSee also:\n* {{msg-mw|Move-page-legend|legend for the form}}\n* {{msg-mw|newtitle|label for new title}}\n* {{msg-mw|Movereason|label for textarea}}\n* {{msg-mw|Movetalk|label for checkbox}}\n* {{msg-mw|Move-leave-redirect|label for checkbox}}\n* {{msg-mw|Fix-double-redirects|label for checkbox}}\n* {{msg-mw|Move-subpages|label for checkbox}}\n* {{msg-mw|Move-watch|label for checkbox}}",
        "movepage-page-exists": "Used as error message when moving page.\n* $1 - page title",
+       "movepage-source-doesnt-exist": "Used as error message when trying to move a page that doesn't exist.\n* $1 - page title",
        "movepage-page-moved": "Used as success message when moving page.\n\nCan be followed by {{msg-mw|Movepage-max-pages}}.\n\nParameters:\n* $1 - old page title (with link)\n* $2 - new page title (with link)\nSee also:\n* {{msg-mw|Movepage-page-unmoved}}",
        "movepage-page-unmoved": "Used as error message when moving page. Parameters:\n* $1 - old page title (with link)\n* $2 - new page title (with link)\nSee also:\n* {{msg-mw|Movepage-page-moved}}",
        "movepage-max-pages": "PROBABLY (A GUESS): when moving a page, you can select an option of moving its subpages, but there is a maximum that can be moved automatically.\n\nParameters:\n* $1 - maximum moved pages, defined in the variable [[mw:Special:MyLanguage/Manual:$wgMaximumMovedPages|$wgMaximumMovedPages]]",
        "delete_and_move_reason": "Shown as reason in content language in the deletion log. Parameter:\n* $1 - The page name for which this page was deleted.",
        "selfmove": "Used as error message when moving page.\n\nSee also:\n* {{msg-mw|badtitletext}}\n* {{msg-mw|immobile-source-namespace}}\n* {{msg-mw|immobile-target-namespace-iw}}\n* {{msg-mw|immobile-target-namespace}}",
        "immobile-source-namespace": "Used as error message. Parameters:\n* $1 - source namespace name\nSee also:\n* {{msg-mw|Immobile-source-page}}\n* {{msg-mw|Immobile-target-namespace}}\n* {{msg-mw|Immobile-target-page}}",
+       "immobile-source-namespace-iw": "Used as error message if somehow something tries to move a page that's on a different wiki.\nSee also:\n* {{msg-mw|Immobile-source-namespace}}\n* {{msg-mw|Immobile-target-namespace-iw}}",
        "immobile-target-namespace": "Used as error message. Parameters:\n* $1 - destination namespace name\nSee also:\n* {{msg-mw|Immobile-source-namespace}}\n* {{msg-mw|Immobile-source-page}}\n* {{msg-mw|Immobile-target-page}}",
        "immobile-target-namespace-iw": "This message appears when attempting to move a page, if a person has typed an interwiki link as a namespace prefix in the input box labelled 'To new title'.  The special page 'Movepage' cannot be used to move a page to another wiki.\n\n'Destination' can be used instead of 'target' in this message.",
        "immobile-source-page": "See also:\n* {{msg-mw|Immobile-source-namespace}}\n* {{msg-mw|Immobile-source-page}}\n* {{msg-mw|Immobile-target-namespace}}\n* {{msg-mw|Immobile-target-page}}",
        "immobile-target-page": "See also:\n* {{msg-mw|Immobile-source-namespace}}\n* {{msg-mw|Immobile-source-page}}\n* {{msg-mw|Immobile-target-namespace}}\n* {{msg-mw|Immobile-target-page}}",
+       "movepage-invalid-target-title": "Error displayed when trying to move a page to an invalid title, e.g., empty or contains prohibited characters.",
        "bad-target-model": "This message is shown when attempting to move a page, but the move would change the page's content model.\nThis may be the case when [[mw:Manual:$wgContentHandlerUseDB|$wgContentHandlerUseDB]] is set to false, because then a page's content model is derived from the page's title.\n\nParameters:\n* $1 - The localized name of the original page's content model:\n**{{msg-mw|Content-model-wikitext}}, {{msg-mw|Content-model-javascript}}, {{msg-mw|Content-model-css}} or {{msg-mw|Content-model-text}}\n* $2 - The localized name of the content model used by the destination title:\n**{{msg-mw|Content-model-wikitext}}, {{msg-mw|Content-model-javascript}}, {{msg-mw|Content-model-css}} or {{msg-mw|Content-model-text}}",
        "imagenocrossnamespace": "Used as error message.\n\nSee also:\n* {{msg-mw|Imagenocrossnamespace}}\n* {{msg-mw|Nonfile-cannot-move-to-file}}",
        "nonfile-cannot-move-to-file": "Used as error message.\n\nSee also:\n* {{msg-mw|Imagenocrossnamespace}}\n* {{msg-mw|Nonfile-cannot-move-to-file}}",
index 286e673..2895fa2 100644 (file)
@@ -210,8 +210,7 @@ class MovePageTest extends MediaWikiTestCase {
                        'Self move' => [
                                'Existent',
                                'Existent',
-                               // @todo There's no reason to return 'articleexists' here
-                               [ [ 'selfmove' ], [ 'articleexists' ] ],
+                               [ [ 'selfmove' ] ],
                        ],
                        'Move to null' => [
                                'Existent',
@@ -221,32 +220,31 @@ class MovePageTest extends MediaWikiTestCase {
                        'Move from empty name' => [
                                Title::makeTitle( NS_MAIN, '' ),
                                'Nonexistent',
-                               // @todo More specific error message
+                               // @todo More specific error message, or make the move valid if the page actually
+                               // exists somehow in the database
                                [ [ 'badarticleerror' ] ],
                        ],
                        'Move to empty name' => [
                                'Existent',
                                Title::makeTitle( NS_MAIN, '' ),
-                               // @todo article-exists is just not correct, and badarticleerror is too general
-                               [ [ 'articleexists' ], [ 'badarticleerror' ] ],
+                               [ [ 'movepage-invalid-target-title' ] ],
                        ],
                        'Move to invalid name' => [
                                'Existent',
                                Title::makeTitle( NS_MAIN, '<' ),
-                               // @todo This is wrong
-                               [],
+                               [ [ 'movepage-invalid-target-title' ] ],
                        ],
                        'Move between invalid names' => [
                                Title::makeTitle( NS_MAIN, '<' ),
                                Title::makeTitle( NS_MAIN, '>' ),
-                               // @todo More specific error message
-                               [ [ 'badarticleerror' ] ],
+                               // @todo First error message should be more specific, or maybe we should make moving
+                               // such pages valid if they actually exist somehow in the database
+                               [ [ 'movepage-source-doesnt-exist' ], [ 'movepage-invalid-target-title' ] ],
                        ],
                        'Move nonexistent' => [
                                'Nonexistent',
                                'Nonexistent2',
-                               // @todo More specific error message
-                               [ [ 'badarticleerror' ] ],
+                               [ [ 'movepage-source-doesnt-exist' ] ],
                        ],
                        'Move over existing' => [
                                'Existent',
@@ -256,20 +254,17 @@ class MovePageTest extends MediaWikiTestCase {
                        'Move from another wiki' => [
                                Title::makeTitle( NS_MAIN, 'Test', '', 'otherwiki' ),
                                'Nonexistent',
-                               // @todo First error is wrong, second is too vague
-                               [ [ 'immobile-source-namespace', '' ], [ 'badarticleerror' ] ],
+                               [ [ 'immobile-source-namespace-iw' ] ],
                        ],
                        'Move special page' => [
                                'Special:FooBar',
                                'Nonexistent',
-                               // @todo Second error not needed
-                               [ [ 'immobile-source-namespace', 'Special' ], [ 'badarticleerror' ] ],
+                               [ [ 'immobile-source-namespace', 'Special' ] ],
                        ],
                        'Move to another wiki' => [
                                'Existent',
                                Title::makeTitle( NS_MAIN, 'Test', '', 'otherwiki' ),
-                               // @todo Second error wrong
-                               [ [ 'immobile-target-namespace-iw' ], [ 'immobile-target-namespace', '' ] ],
+                               [ [ 'immobile-target-namespace-iw' ] ],
                        ],
                        'Move to special page' =>
                                [ 'Existent', 'Special:FooBar', [ [ 'immobile-target-namespace', 'Special' ] ] ],
@@ -300,8 +295,7 @@ class MovePageTest extends MediaWikiTestCase {
                        'File to non-file' => [
                                'File:Existent.jpg',
                                'Nonexistent',
-                               // @todo First error not needed
-                               [ [ 'imagetypemismatch' ], [ 'imagenocrossnamespace' ] ],
+                               [ [ 'imagenocrossnamespace' ] ],
                        ],
                        'Existing file to non-existing file' => [
                                'File:Existent.jpg',