From ba7287fce6d64b72916ae7fe3f3a31c5185347d2 Mon Sep 17 00:00:00 2001 From: Umherirrender Date: Sat, 18 Aug 2018 04:37:59 +0200 Subject: [PATCH] Give more specific error messages on Special:Redirect Added some basic tests Bug: T202183 Change-Id: Ib0dd50ff5575a2b2093a57afce79e9f8623fa24d (cherry picked from commit 114e6547dea1a2508fe24889d65221af0163622a) --- includes/specials/SpecialRedirect.php | 85 +++++++++++-------- languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + .../includes/specials/SpecialRedirectTest.php | 70 +++++++++++++++ 4 files changed, 120 insertions(+), 37 deletions(-) create mode 100644 tests/phpunit/includes/specials/SpecialRedirectTest.php diff --git a/includes/specials/SpecialRedirect.php b/includes/specials/SpecialRedirect.php index 800604876f..064616b5d8 100644 --- a/includes/specials/SpecialRedirect.php +++ b/includes/specials/SpecialRedirect.php @@ -68,16 +68,18 @@ class SpecialRedirect extends FormSpecialPage { /** * Handle Special:Redirect/user/xxxx (by redirecting to User:YYYY) * - * @return string|null Url to redirect to, or null if $mValue is invalid. + * @return Status A good status contains the url to redirect to */ function dispatchUser() { if ( !ctype_digit( $this->mValue ) ) { - return null; + // Message: redirect-not-numeric + return Status::newFatal( $this->getMessagePrefix() . '-not-numeric' ); } $user = User::newFromId( (int)$this->mValue ); $username = $user->getName(); // load User as side-effect if ( $user->isAnon() ) { - return null; + // Message: redirect-not-exists + return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } if ( $user->isHidden() && !MediaWikiServices::getInstance()->getPermissionManager() ->userHasRight( $this->getUser(), 'hideuser' ) @@ -86,24 +88,29 @@ class SpecialRedirect extends FormSpecialPage { } $userpage = Title::makeTitle( NS_USER, $username ); - return $userpage->getFullURL( '', false, PROTO_CURRENT ); + return Status::newGood( $userpage->getFullURL( '', false, PROTO_CURRENT ) ); } /** * Handle Special:Redirect/file/xxxx * - * @return string|null Url to redirect to, or null if $mValue is not found. + * @return Status A good status contains the url to redirect to */ function dispatchFile() { - $title = Title::makeTitleSafe( NS_FILE, $this->mValue ); - - if ( !$title instanceof Title ) { - return null; + try { + $title = Title::newFromTextThrow( $this->mValue, NS_FILE ); + if ( $title && !$title->inNamespace( NS_FILE ) ) { + // If the given value contains a namespace enforce file namespace + $title = Title::newFromTextThrow( Title::makeName( NS_FILE, $this->mValue ) ); + } + } catch ( MalformedTitleException $e ) { + return Status::newFatal( $e->getMessageObject() ); } $file = wfFindFile( $title ); if ( !$file || !$file->exists() ) { - return null; + // Message: redirect-not-exists + return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } // Default behavior: Use the direct link to the file. $url = $file->getUrl(); @@ -121,48 +128,52 @@ class SpecialRedirect extends FormSpecialPage { } } - return $url; + return Status::newGood( $url ); } /** * Handle Special:Redirect/revision/xxx * (by redirecting to index.php?oldid=xxx) * - * @return string|null Url to redirect to, or null if $mValue is invalid. + * @return Status A good status contains the url to redirect to */ function dispatchRevision() { $oldid = $this->mValue; if ( !ctype_digit( $oldid ) ) { - return null; + // Message: redirect-not-numeric + return Status::newFatal( $this->getMessagePrefix() . '-not-numeric' ); } $oldid = (int)$oldid; if ( $oldid === 0 ) { - return null; + // Message: redirect-not-exists + return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } - return wfAppendQuery( wfScript( 'index' ), [ + return Status::newGood( wfAppendQuery( wfScript( 'index' ), [ 'oldid' => $oldid - ] ); + ] ) ); } /** * Handle Special:Redirect/page/xxx (by redirecting to index.php?curid=xxx) * - * @return string|null Url to redirect to, or null if $mValue is invalid. + * @return Status A good status contains the url to redirect to */ function dispatchPage() { $curid = $this->mValue; if ( !ctype_digit( $curid ) ) { - return null; + // Message: redirect-not-numeric + return Status::newFatal( $this->getMessagePrefix() . '-not-numeric' ); } $curid = (int)$curid; if ( $curid === 0 ) { - return null; + // Message: redirect-not-exists + return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } - return wfAppendQuery( wfScript( 'index' ), [ + return Status::newGood( wfAppendQuery( wfScript( 'index' ), [ 'curid' => $curid - ] ); + ] ) ); } /** @@ -170,19 +181,21 @@ class SpecialRedirect extends FormSpecialPage { * (by redirecting to index.php?title=Special:Log&logid=xxx) * * @since 1.27 - * @return string|null Url to redirect to, or null if $mValue is invalid. + * @return Status A good status contains the url to redirect to */ function dispatchLog() { $logid = $this->mValue; if ( !ctype_digit( $logid ) ) { - return null; + // Message: redirect-not-numeric + return Status::newFatal( $this->getMessagePrefix() . '-not-numeric' ); } $logid = (int)$logid; if ( $logid === 0 ) { - return null; + // Message: redirect-not-exists + return Status::newFatal( $this->getMessagePrefix() . '-not-exists' ); } $query = [ 'title' => 'Special:Log', 'logid' => $logid ]; - return wfAppendQuery( wfScript( 'index' ), $query ); + return Status::newGood( wfAppendQuery( wfScript( 'index' ), $query ) ); } /** @@ -191,41 +204,39 @@ class SpecialRedirect extends FormSpecialPage { * or do nothing (if $mValue wasn't set) allowing the form to be * displayed. * - * @return bool True if a redirect was successfully handled. + * @return Status|bool True if a redirect was successfully handled. */ function dispatch() { // the various namespaces supported by Special:Redirect switch ( $this->mType ) { case 'user': - $url = $this->dispatchUser(); + $status = $this->dispatchUser(); break; case 'file': - $url = $this->dispatchFile(); + $status = $this->dispatchFile(); break; case 'revision': - $url = $this->dispatchRevision(); + $status = $this->dispatchRevision(); break; case 'page': - $url = $this->dispatchPage(); + $status = $this->dispatchPage(); break; case 'logid': - $url = $this->dispatchLog(); + $status = $this->dispatchLog(); break; default: - $url = null; + $status = null; break; } - if ( $url ) { - $this->getOutput()->redirect( $url ); + if ( $status && $status->isGood() ) { + $this->getOutput()->redirect( $status->getValue() ); return true; } if ( !is_null( $this->mValue ) ) { $this->getOutput()->setStatusCode( 404 ); - // Message: redirect-not-exists - $msg = $this->getMessagePrefix() . '-not-exists'; - return Status::newFatal( $msg ); + return $status; } return false; diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 1c11deb4e4..86762384a8 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -3869,6 +3869,7 @@ "redirect-file": "Filename", "redirect-logid": "Log ID", "redirect-not-exists": "Value not found", + "redirect-not-numeric": "Value not numeric", "fileduplicatesearch": "Search for duplicate files", "fileduplicatesearch-summary": "Search for duplicate files based on hash values.", "fileduplicatesearch-filename": "Filename:", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index ba5bee606c..576c2bf241 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -4066,6 +4066,7 @@ "redirect-file": "Description of lookup type for [[Special:Redirect]].\n{{Identical|Filename}}", "redirect-logid": "Description of lookup type for [[Special:Redirect]].\n{{Identical|Log ID}}", "redirect-not-exists": "Used as error message in [[Special:Redirect]]", + "redirect-not-numeric": "Used as error message in [[Special:Redirect]]", "fileduplicatesearch": "Name of special page [[Special:FileDuplicateSearch]].", "fileduplicatesearch-summary": "Summary of [[Special:FileDuplicateSearch]]", "fileduplicatesearch-filename": "Input form of [[Special:FileDuplicateSearch]]:\n\n{{Identical|Filename}}", diff --git a/tests/phpunit/includes/specials/SpecialRedirectTest.php b/tests/phpunit/includes/specials/SpecialRedirectTest.php new file mode 100644 index 0000000000..bee35dfd81 --- /dev/null +++ b/tests/phpunit/includes/specials/SpecialRedirectTest.php @@ -0,0 +1,70 @@ +addToDatabase(); + $value = $user->getId(); + } + + $page->setParameter( $type . '/' . $value ); + + $status = $page->$method(); + $this->assertSame( + $status->isGood(), $expectedStatus === 'good', + $method . ' does not return expected status "' . $expectedStatus . '"' + ); + } + + public static function provideDispatch() { + foreach ( [ + [ 'nonumeric', 'fatal' ], + [ '3', 'fatal' ], + [ self::CREATE_USER, 'good' ], + ] as $dispatchUser ) { + yield [ 'dispatchUser', 'user', $dispatchUser[0], $dispatchUser[1] ]; + } + foreach ( [ + [ 'bad