MediaWiki.php: Factor out tryNormaliseRedirect
authorTimo Tijhof <krinklemail@gmail.com>
Fri, 19 Jun 2015 19:56:36 +0000 (20:56 +0100)
committerTimo Tijhof <krinklemail@gmail.com>
Tue, 23 Jun 2015 06:18:35 +0000 (07:18 +0100)
This is in preparation for fixing T67402, which requires adding
logic inside this condition block. However the to-be-added code
will influences whether or not a redirect should be made.

In case a redirect is not made, it has to fall through to the next
'elseif' handler in MediaWiki::performRequest(), which is not possible
from inside the 'if' block.

Hence, move it out in a separate block and use a boolean return value
to communicate whether the case has been handled.

This also allows us to unit test this thing. Which is desperately
needed. Albeit ugly as it requires lots of mocking.

Change-Id: If3157f2ff1fd3ab2ca20a5d1f550d864ea62c493

includes/MediaWiki.php
tests/phpunit/includes/MediaWikiTest.php [new file with mode: 0644]

index 7a0d7b7..932dea2 100644 (file)
@@ -239,63 +239,97 @@ class MediaWiki {
                                }
                                throw new BadTitleError();
                        }
-               // Redirect loops, no title in URL, $wgUsePathInfo URLs, and URLs with a variant
-               } elseif ( $request->getVal( 'action', 'view' ) == 'view' && !$request->wasPosted()
-                       && ( $request->getVal( 'title' ) === null
-                               || $title->getPrefixedDBkey() != $request->getVal( 'title' ) )
-                       && !count( $request->getValueNames( array( 'action', 'title' ) ) )
-                       && Hooks::run( 'TestCanonicalRedirect', array( $request, $title, $output ) )
-               ) {
-                       if ( $title->isSpecialPage() ) {
-                               list( $name, $subpage ) = SpecialPageFactory::resolveAlias( $title->getDBkey() );
-                               if ( $name ) {
-                                       $title = SpecialPage::getTitleFor( $name, $subpage );
-                               }
-                       }
-                       $targetUrl = wfExpandUrl( $title->getFullURL(), PROTO_CURRENT );
-                       // Redirect to canonical url, make it a 301 to allow caching
-                       if ( $targetUrl == $request->getFullRequestURL() ) {
-                               $message = "Redirect loop detected!\n\n" .
-                                       "This means the wiki got confused about what page was " .
-                                       "requested; this sometimes happens when moving a wiki " .
-                                       "to a new server or changing the server configuration.\n\n";
-
-                               if ( $this->config->get( 'UsePathInfo' ) ) {
-                                       $message .= "The wiki is trying to interpret the page " .
-                                               "title from the URL path portion (PATH_INFO), which " .
-                                               "sometimes fails depending on the web server. Try " .
-                                               "setting \"\$wgUsePathInfo = false;\" in your " .
-                                               "LocalSettings.php, or check that \$wgArticlePath " .
-                                               "is correct.";
+               // Handle any other redirects.
+               // Redirect loops, titleless URL, $wgUsePathInfo URLs, and URLs with a variant
+               } elseif ( !$this->tryNormaliseRedirect( $title ) ) {
+
+                       // Special pages
+                       if ( NS_SPECIAL == $title->getNamespace() ) {
+                               // Actions that need to be made when we have a special pages
+                               SpecialPageFactory::executePath( $title, $this->context );
+                       } else {
+                               // ...otherwise treat it as an article view. The article
+                               // may still be a wikipage redirect to another article or URL.
+                               $article = $this->initializeArticle();
+                               if ( is_object( $article ) ) {
+                                       $this->performAction( $article, $requestTitle );
+                               } elseif ( is_string( $article ) ) {
+                                       $output->redirect( $article );
                                } else {
-                                       $message .= "Your web server was detected as possibly not " .
-                                               "supporting URL path components (PATH_INFO) correctly; " .
-                                               "check your LocalSettings.php for a customized " .
-                                               "\$wgArticlePath setting and/or toggle \$wgUsePathInfo " .
-                                               "to true.";
+                                       throw new MWException( "Shouldn't happen: MediaWiki::initializeArticle()"
+                                               . " returned neither an object nor a URL" );
                                }
-                               throw new HttpError( 500, $message );
-                       } else {
-                               $output->setSquidMaxage( 1200 );
-                               $output->redirect( $targetUrl, '301' );
                        }
-               // Special pages
-               } elseif ( NS_SPECIAL == $title->getNamespace() ) {
-                       // Actions that need to be made when we have a special pages
-                       SpecialPageFactory::executePath( $title, $this->context );
-               } else {
-                       // ...otherwise treat it as an article view. The article
-                       // may be a redirect to another article or URL.
-                       $article = $this->initializeArticle();
-                       if ( is_object( $article ) ) {
-                               $this->performAction( $article, $requestTitle );
-                       } elseif ( is_string( $article ) ) {
-                               $output->redirect( $article );
+               }
+       }
+
+       /**
+        * Handle redirects for uncanonical title requests.
+        *
+        * Handles:
+        * - Redirect loops.
+        * - No title in URL.
+        * - $wgUsePathInfo URLs.
+        * - URLs with a variant.
+        * - Other non-standard URLs (as long as they have no extra query parameters).
+        *
+        * Behaviour:
+        * - Normalise title values:
+        *   /wiki/Foo%20Bar -> /wiki/Foo_Bar
+        * - Normalise empty title:
+        *   /wiki/ -> /wiki/Main
+        *   /w/index.php?title= -> /wiki/Main
+        * - Don't redirect anything with query parameters other than 'title' or 'action=view'.
+        *
+        * @return bool True if a redirect was set.
+        */
+       private function tryNormaliseRedirect( $title ) {
+               $request = $this->context->getRequest();
+               $output = $this->context->getOutput();
+
+               if ( $request->getVal( 'action', 'view' ) != 'view'
+                       || $request->wasPosted()
+                       || ( $request->getVal( 'title' ) !== null
+                               && $title->getPrefixedDBkey() == $request->getVal( 'title' ) )
+                       || count( $request->getValueNames( array( 'action', 'title' ) ) )
+                       || !Hooks::run( 'TestCanonicalRedirect', array( $request, $title, $output ) )
+               ) {
+                       return false;
+               }
+
+               if ( $title->isSpecialPage() ) {
+                       list( $name, $subpage ) = SpecialPageFactory::resolveAlias( $title->getDBkey() );
+                       if ( $name ) {
+                               $title = SpecialPage::getTitleFor( $name, $subpage );
+                       }
+               }
+               // Redirect to canonical url, make it a 301 to allow caching
+               $targetUrl = wfExpandUrl( $title->getFullURL(), PROTO_CURRENT );
+               if ( $targetUrl == $request->getFullRequestURL() ) {
+                       $message = "Redirect loop detected!\n\n" .
+                               "This means the wiki got confused about what page was " .
+                               "requested; this sometimes happens when moving a wiki " .
+                               "to a new server or changing the server configuration.\n\n";
+
+                       if ( $this->config->get( 'UsePathInfo' ) ) {
+                               $message .= "The wiki is trying to interpret the page " .
+                                       "title from the URL path portion (PATH_INFO), which " .
+                                       "sometimes fails depending on the web server. Try " .
+                                       "setting \"\$wgUsePathInfo = false;\" in your " .
+                                       "LocalSettings.php, or check that \$wgArticlePath " .
+                                       "is correct.";
                        } else {
-                               throw new MWException( "Shouldn't happen: MediaWiki::initializeArticle()"
-                                       . " returned neither an object nor a URL" );
+                               $message .= "Your web server was detected as possibly not " .
+                                       "supporting URL path components (PATH_INFO) correctly; " .
+                                       "check your LocalSettings.php for a customized " .
+                                       "\$wgArticlePath setting and/or toggle \$wgUsePathInfo " .
+                                       "to true.";
                        }
+                       throw new HttpError( 500, $message );
                }
+               $output->setSquidMaxage( 1200 );
+               $output->redirect( $targetUrl, '301' );
+               return true;
        }
 
        /**
diff --git a/tests/phpunit/includes/MediaWikiTest.php b/tests/phpunit/includes/MediaWikiTest.php
new file mode 100644 (file)
index 0000000..df94d3e
--- /dev/null
@@ -0,0 +1,157 @@
+<?php
+
+class MediaWikiTest extends MediaWikiTestCase {
+       protected function setUp() {
+               parent::setUp();
+
+               $this->setMwGlobals( array(
+                       'wgServer' => 'http://example.org',
+                       'wgScriptPath' => '/w',
+                       'wgScript' => '/w/index.php',
+                       'wgArticlePath' => '/wiki/$1',
+                       'wgActionPaths' => array(),
+               ) );
+       }
+
+       public static function provideTryNormaliseRedirect() {
+               return array(
+                       array(
+                               // View: Canonical
+                               'url' => 'http://example.org/wiki/Foo_Bar',
+                               'query' => array(),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // View: Escaped title
+                               'url' => 'http://example.org/wiki/Foo%20Bar',
+                               'query' => array(),
+                               'title' => 'Foo_Bar',
+                               'redirect' => 'http://example.org/wiki/Foo_Bar',
+                       ),
+                       array(
+                               // View: Script path
+                               'url' => 'http://example.org/w/index.php?title=Foo_Bar',
+                               'query' => array( 'title' => 'Foo_Bar' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // View: Script path with implicit title from page id
+                               'url' => 'http://example.org/w/index.php?curid=123',
+                               'query' => array( 'curid' => '123' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // View: Script path with implicit title from revision id
+                               'url' => 'http://example.org/w/index.php?oldid=123',
+                               'query' => array( 'oldid' => '123' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // View: Script path without title
+                               'url' => 'http://example.org/w/index.php',
+                               'query' => array(),
+                               'title' => 'Main_Page',
+                               'redirect' => 'http://example.org/wiki/Main_Page',
+                       ),
+                       array(
+                               // View: Script path with empty title
+                               'url' => 'http://example.org/w/index.php?title=',
+                               'query' => array( 'title' => '' ),
+                               'title' => 'Main_Page',
+                               'redirect' => 'http://example.org/wiki/Main_Page',
+                       ),
+                       array(
+                               // View: Index with escaped title
+                               'url' => 'http://example.org/w/index.php?title=Foo%20Bar',
+                               'query' => array( 'title' => 'Foo Bar' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => 'http://example.org/wiki/Foo_Bar',
+                       ),
+                       array(
+                               // View: Script path with escaped title
+                               'url' => 'http://example.org/w/?title=Foo_Bar',
+                               'query' => array( 'title' => 'Foo_Bar' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // View: Root path with escaped title
+                               'url' => 'http://example.org/?title=Foo_Bar',
+                               'query' => array( 'title' => 'Foo_Bar' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // View: Canonical with redundant query
+                               'url' => 'http://example.org/wiki/Foo_Bar?action=view',
+                               'query' => array( 'action' => 'view' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // Edit: Canonical view url with action query
+                               'url' => 'http://example.org/wiki/Foo_Bar?action=edit',
+                               'query' => array( 'action' => 'edit' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // View: Index with action query
+                               'url' => 'http://example.org/w/index.php?title=Foo_Bar&action=view',
+                               'query' => array( 'title' => 'Foo_Bar', 'action' => 'view' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+                       array(
+                               // Edit: Index with action query
+                               'url' => 'http://example.org/w/index.php?title=Foo_Bar&action=edit',
+                               'query' => array( 'title' => 'Foo_Bar', 'action' => 'edit' ),
+                               'title' => 'Foo_Bar',
+                               'redirect' => false,
+                       ),
+               );
+       }
+
+       /**
+        * @dataProvider provideTryNormaliseRedirect
+        * @covers MediaWiki::tryNormaliseRedirect
+        */
+       public function testTryNormaliseRedirect( $url, $query, $title, $expectedRedirect = false ) {
+               // Set SERVER because interpolateTitle() doesn't use getRequestURL(),
+               // whereas tryNormaliseRedirect does().
+               $_SERVER['REQUEST_URI'] = $url;
+
+               $req = new FauxRequest( $query );
+               $req->setRequestURL( $url );
+               // This adds a virtual 'title' query parameter. Normally called from Setup.php
+               $req->interpolateTitle();
+
+               $titleObj = Title::newFromText( $title );
+
+               // Set global context since some involved code paths don't yet have context
+               $context = RequestContext::getMain();
+               $context->setRequest( $req );
+               $context->setTitle( $titleObj );
+
+               $mw = new MediaWiki( $context );
+
+               $method = new ReflectionMethod( $mw, 'tryNormaliseRedirect' );
+               $method->setAccessible( true );
+               $ret = $method->invoke( $mw, $titleObj );
+
+               $this->assertEquals(
+                       $expectedRedirect !== false,
+                       $ret,
+                       'Return true only when redirecting'
+               );
+
+               $this->assertEquals(
+                       $expectedRedirect ?: '',
+                       $context->getOutput()->getRedirect()
+               );
+       }
+}