Give more specific error messages on Special:Redirect

Added some basic tests

Bug: T202183
Change-Id: Ib0dd50ff5575a2b2093a57afce79e9f8623fa24d

diff --git a/includes/specials/SpecialRedirect.php b/includes/specials/SpecialRedirect.php
index e827911..1b2bda9 100644 (file)
--- a/includes/specials/SpecialRedirect.php
+++ b/includes/specials/SpecialRedirect.php
@@ -68,37 +68,44 @@ 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' );
                }
                $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();
@@ -116,48 +123,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
-               ] );
+               ] ) );
        }
 
        /**
@@ -165,19 +176,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 ) );
        }
 
        /**
@@ -186,41 +199,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 47747b6..1a3479d 100644 (file)
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -3894,6 +3894,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 4b1717d..dd3d6bb 100644 (file)
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -4094,6 +4094,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 (file)
index 0000000..bee35df
--- /dev/null
+++ b/tests/phpunit/includes/specials/SpecialRedirectTest.php
@@ -0,0 +1,70 @@
+<?php
+
+/**
+ * Test class for SpecialRedirect class
+ *
+ * @since 1.32
+ *
+ * @license GPL-2.0-or-later
+ * @group Database
+ */
+class SpecialRedirectTest extends MediaWikiTestCase {
+
+       protected $tablesUsed = [ 'user' ];
+
+       const CREATE_USER = 'create_user';
+
+       /**
+        * @dataProvider provideDispatch
+        * @covers SpecialRedirect::dispatchUser()
+        * @covers SpecialRedirect::dispatchFile()
+        * @covers SpecialRedirect::dispatchRevision()
+        * @covers SpecialRedirect::dispatchPage()
+        * @covers SpecialRedirect::dispatchLog()
+        */
+       public function testDispatch( $method, $type, $value, $expectedStatus ) {
+               $page = new SpecialRedirect();
+
+               // setup the user object
+               if ( $value === self::CREATE_USER ) {
+                       $user = User::newFromName( __CLASS__ );
+                       $user->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<name', 'fatal' ],
+                       [ 'File:Non-exists.jpg', 'fatal' ],
+                       // TODO Cannot test the good path here, because a file must exists
+               ] as $dispatchFile ) {
+                       yield [ 'dispatchFile', 'file', $dispatchFile[0], $dispatchFile[1] ];
+               }
+               foreach ( [
+                       [ 'nonumeric', 'fatal' ],
+                       [ '0', 'fatal' ],
+                       [ '1', 'good' ],
+               ] as $dispatch ) {
+                       yield [ 'dispatchRevision', 'revision', $dispatch[0], $dispatch[1] ];
+                       yield [ 'dispatchPage', 'revision', $dispatch[0], $dispatch[1] ];
+                       yield [ 'dispatchLog', 'log', $dispatch[0], $dispatch[1] ];
+               }
+       }
+
+}