Title: ensure getBaseTitle and getRootTitle return valid Titles

Since getBaseText() and getRootText() may return text with trailing
whitespace, getBaseTitle and getRootTitle must use makeTitleSafe instead
of makeTitle.

Bug: T225585
Change-Id: Id92df552f05e6a9ed7c9259a8779fa94c3587a3e

diff --git a/includes/Title.php b/includes/Title.php
index f69f1a4..b27baa8 100644 (file)
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -23,6 +23,7 @@
  */
 
 use MediaWiki\Permissions\PermissionManager;
+use Wikimedia\Assert\Assert;
 use Wikimedia\Rdbms\Database;
 use Wikimedia\Rdbms\IDatabase;
 use MediaWiki\Linker\LinkTarget;
@@ -851,7 +852,10 @@ class Title implements LinkTarget, IDBAccessObject {
        /**
         * Returns true if the title is valid, false if it is invalid.
         *
-        * Valid titles can be round-tripped via makeTitleSafe() and newFromText().
+        * Valid titles can be round-tripped via makeTitle() and newFromText().
+        * Their DB key can be used in the database, though it may not have the correct
+        * capitalization.
+        *
         * Invalid titles may get returned from makeTitle(), and it may be useful to
         * allow them to exist, e.g. in order to process log entries about pages in
         * namespaces that belong to extensions that are no longer installed.
@@ -870,10 +874,23 @@ class Title implements LinkTarget, IDBAccessObject {
 
                try {
                        $services->getTitleParser()->parseTitle( $this->mDbkeyform, $this->mNamespace );
-                       return true;
                } catch ( MalformedTitleException $ex ) {
                        return false;
                }
+
+               try {
+                       // Title value applies basic syntax checks. Should perhaps be moved elsewhere.
+                       new TitleValue(
+                               $this->mNamespace,
+                               $this->mDbkeyform,
+                               $this->mFragment,
+                               $this->mInterwiki
+                       );
+               } catch ( InvalidArgumentException $ex ) {
+                       return false;
+               }
+
+               return true;
        }
 
        /**
@@ -1728,6 +1745,9 @@ class Title implements LinkTarget, IDBAccessObject {
        /**
         * Get the root page name text without a namespace, i.e. the leftmost part before any slashes
         *
+        * @note the return value may contain trailing whitespace and is thus
+        * not safe for use with makeTitle or TitleValue.
+        *
         * @par Example:
         * @code
         * Title::newFromText('User:Foo/Bar/Baz')->getRootText();
@@ -1761,12 +1781,20 @@ class Title implements LinkTarget, IDBAccessObject {
         * @since 1.20
         */
        public function getRootTitle() {
-               return self::makeTitle( $this->mNamespace, $this->getRootText() );
+               $title = self::makeTitleSafe( $this->mNamespace, $this->getRootText() );
+               Assert::postcondition(
+                       $title !== null,
+                       'makeTitleSafe() should always return a Title for the text returned by getRootText().'
+               );
+               return $title;
        }
 
        /**
         * Get the base page name without a namespace, i.e. the part before the subpage name
         *
+        * @note the return value may contain trailing whitespace and is thus
+        * not safe for use with makeTitle or TitleValue.
+        *
         * @par Example:
         * @code
         * Title::newFromText('User:Foo/Bar/Baz')->getBaseText();
@@ -1794,7 +1822,7 @@ class Title implements LinkTarget, IDBAccessObject {
        }
 
        /**
-        * Get the base page name title, i.e. the part before the subpage name
+        * Get the base page name title, i.e. the part before the subpage name.
         *
         * @par Example:
         * @code
@@ -1806,7 +1834,12 @@ class Title implements LinkTarget, IDBAccessObject {
         * @since 1.20
         */
        public function getBaseTitle() {
-               return self::makeTitle( $this->mNamespace, $this->getBaseText() );
+               $title = self::makeTitleSafe( $this->mNamespace, $this->getBaseText() );
+               Assert::postcondition(
+                       $title !== null,
+                       'makeTitleSafe() should always return a Title for the text returned by getBaseText().'
+               );
+               return $title;
        }
 
        /**

diff --git a/tests/phpunit/includes/TitleTest.php b/tests/phpunit/includes/TitleTest.php
index 225a786..9b021c4 100644 (file)
--- a/tests/phpunit/includes/TitleTest.php
+++ b/tests/phpunit/includes/TitleTest.php
@@ -494,17 +494,31 @@ class TitleTest extends MediaWikiTestCase {
         */
        public function testGetBaseText( $title, $expected, $msg = '' ) {
                $title = Title::newFromText( $title );
-               $this->assertEquals( $expected,
+               $this->assertSame( $expected,
                        $title->getBaseText(),
                        $msg
                );
        }
 
+       /**
+        * @dataProvider provideBaseTitleCases
+        * @covers Title::getBaseTitle
+        */
+       public function testGetBaseTitle( $title, $expected, $msg = '' ) {
+               $title = Title::newFromText( $title );
+               $base = $title->getBaseTitle();
+               $this->assertTrue( $base->isValid(), $msg );
+               $this->assertTrue(
+                       $base->equals( Title::makeTitleSafe( $title->getNamespace(), $expected ) ),
+                       $msg
+               );
+       }
+
        public static function provideBaseTitleCases() {
                return [
                        # Title, expected base, optional message
                        [ 'User:John_Doe/subOne/subTwo', 'John Doe/subOne' ],
-                       [ 'User:Foo/Bar/Baz', 'Foo/Bar' ],
+                       [ 'User:Foo / Bar / Baz', 'Foo / Bar ' ],
                ];
        }
 
@@ -520,11 +534,25 @@ class TitleTest extends MediaWikiTestCase {
                );
        }
 
+       /**
+        * @dataProvider provideRootTitleCases
+        * @covers Title::getRootTitle
+        */
+       public function testGetRootTitle( $title, $expected, $msg = '' ) {
+               $title = Title::newFromText( $title );
+               $root = $title->getRootTitle();
+               $this->assertTrue( $root->isValid(), $msg );
+               $this->assertTrue(
+                       $root->equals( Title::makeTitleSafe( $title->getNamespace(), $expected ) ),
+                       $msg
+               );
+       }
+
        public static function provideRootTitleCases() {
                return [
                        # Title, expected base, optional message
                        [ 'User:John_Doe/subOne/subTwo', 'John Doe' ],
-                       [ 'User:Foo/Bar/Baz', 'Foo' ],
+                       [ 'User:Foo / Bar / Baz', 'Foo ' ],
                ];
        }
 
@@ -709,6 +737,12 @@ class TitleTest extends MediaWikiTestCase {
                        [ Title::makeTitle( NS_MAIN, '|' ), false ],
                        [ Title::makeTitle( NS_MAIN, '#' ), false ],
                        [ Title::makeTitle( NS_MAIN, 'Test' ), true ],
+                       [ Title::makeTitle( NS_MAIN, ' Test' ), false ],
+                       [ Title::makeTitle( NS_MAIN, '_Test' ), false ],
+                       [ Title::makeTitle( NS_MAIN, 'Test ' ), false ],
+                       [ Title::makeTitle( NS_MAIN, 'Test_' ), false ],
+                       [ Title::makeTitle( NS_MAIN, "Test\nthis" ), false ],
+                       [ Title::makeTitle( NS_MAIN, "Test\tthis" ), false ],
                        [ Title::makeTitle( -33, 'Test' ), false ],
                        [ Title::makeTitle( 77663399, 'Test' ), false ],
                ];