From: Aryeh Gregor Date: Mon, 6 May 2019 11:14:24 +0000 (+0300) Subject: Return better errors from MovePage::isValidMove() X-Git-Tag: 1.34.0-rc.0~674 X-Git-Url: https://git.heureux-cyclage.org/?a=commitdiff_plain;h=4f912600c0a9dbf0c74c0498f33909cab8a1de6f;p=lhc%2Fweb%2Fwiklou.git Return better errors from MovePage::isValidMove() Previously the errors returned were incorrect or redundant in a number of cases. Change-Id: Ief96e69b0ae09afb9642f9ed93b2419a36351292 --- diff --git a/includes/MovePage.php b/includes/MovePage.php index 578c96d177..a63eeaebda 100644 --- a/includes/MovePage.php +++ b/includes/MovePage.php @@ -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; } diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 7bcda5bd41..56d0842e7c 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -2817,6 +2817,7 @@ "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.", @@ -2835,10 +2836,12 @@ "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.", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index a2ee29a66e..ae6379b840 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -3026,6 +3026,7 @@ "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]]", @@ -3044,10 +3045,12 @@ "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}}", diff --git a/tests/phpunit/includes/MovePageTest.php b/tests/phpunit/includes/MovePageTest.php index 286e6734a3..2895fa286c 100644 --- a/tests/phpunit/includes/MovePageTest.php +++ b/tests/phpunit/includes/MovePageTest.php @@ -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',