Ensure canHaveTalkPage returns false when getTalkPage would fail.
authordaniel <dkinzler@wikimedia.org>
Mon, 3 Jun 2019 22:56:22 +0000 (00:56 +0200)
committerdaniel <dkinzler@wikimedia.org>
Wed, 3 Jul 2019 08:40:10 +0000 (10:40 +0200)
This causes Title::getTalkPage and NamespaceInfo::getTitle() to throw
an MWException when called on a LinkTarget that is an interwiki link
or a relative section link. These methods were already throwing
MWException when called on a link to a Special page.

Bug: T224814
Change-Id: I525c186a5b8b8fc22bca195da48afead3bfbd402

includes/Title.php
includes/title/NamespaceInfo.php
tests/phpunit/includes/TitleTest.php
tests/phpunit/includes/parser/CoreParserFunctionsTest.php
tests/phpunit/includes/title/NamespaceInfoTest.php

index b27baa8..6e75102 100644 (file)
@@ -1145,14 +1145,16 @@ class Title implements LinkTarget, IDBAccessObject {
        /**
         * Can this title have a corresponding talk page?
         *
-        * @see NamespaceInfo::hasTalkNamespace
+        * False for relative section links (with getText() === ''),
+        * interwiki links (with getInterwiki() !== ''), and pages in NS_SPECIAL.
+        *
+        * @see NamespaceInfo::canHaveTalkPage
         * @since 1.30
         *
         * @return bool True if this title either is a talk page or can have a talk page associated.
         */
        public function canHaveTalkPage() {
-               return MediaWikiServices::getInstance()->getNamespaceInfo()->
-                       hasTalkNamespace( $this->mNamespace );
+               return MediaWikiServices::getInstance()->getNamespaceInfo()->canHaveTalkPage( $this );
        }
 
        /**
@@ -1167,11 +1169,15 @@ class Title implements LinkTarget, IDBAccessObject {
        /**
         * Can this title be added to a user's watchlist?
         *
+        * False for relative section links (with getText() === ''),
+        * interwiki links (with getInterwiki() !== ''), and pages in NS_SPECIAL.
+        *
         * @return bool
         */
        public function isWatchable() {
-               return !$this->isExternal() && MediaWikiServices::getInstance()->getNamespaceInfo()->
-                       isWatchable( $this->mNamespace );
+               $nsInfo = MediaWikiServices::getInstance()->getNamespaceInfo();
+               return $this->getText() !== '' && !$this->isExternal() &&
+                       $nsInfo->isWatchable( $this->mNamespace );
        }
 
        /**
@@ -1532,8 +1538,11 @@ class Title implements LinkTarget, IDBAccessObject {
        /**
         * Get a Title object associated with the talk page of this article
         *
-        * @deprecated since 1.34, use NamespaceInfo::getTalkPage
+        * @deprecated since 1.34, use getTalkPageIfDefined() or NamespaceInfo::getTalkPage()
+        *             with NamespaceInfo::canHaveTalkPage().
         * @return Title The object for the talk page
+        * @throws MWException if $target doesn't have talk pages, e.g. because it's in NS_SPECIAL
+        *         or because it's a relative link, or an interwiki link.
         */
        public function getTalkPage() {
                return self::castFromLinkTarget(
index cdb8f25..2ed8729 100644 (file)
@@ -142,6 +142,8 @@ class NamespaceInfo {
         *
         * @param int $index Namespace index
         * @return int
+        * @throws MWException if the given namespace doesn't have an associated talk namespace
+        *         (e.g. NS_SPECIAL).
         */
        public function getTalk( $index ) {
                $this->isMethodValidFor( $index, __METHOD__ );
@@ -151,15 +153,52 @@ class NamespaceInfo {
        }
 
        /**
+        * Get a LinkTarget referring to the talk page of $target.
+        *
+        * @see canHaveTalkPage
         * @param LinkTarget $target
         * @return LinkTarget Talk page for $target
-        * @throws MWException if $target's namespace doesn't have talk pages (e.g., NS_SPECIAL)
+        * @throws MWException if $target doesn't have talk pages, e.g. because it's in NS_SPECIAL,
+        *         because it's a relative section-only link, or it's an an interwiki link.
         */
        public function getTalkPage( LinkTarget $target ) : LinkTarget {
+               if ( $target->getText() === '' ) {
+                       throw new MWException( 'Can\'t determine talk page associated with relative section link' );
+               }
+
+               if ( $target->getInterwiki() !== '' ) {
+                       throw new MWException( 'Can\'t determine talk page associated with interwiki link' );
+               }
+
                if ( $this->isTalk( $target->getNamespace() ) ) {
                        return $target;
                }
-               return new TitleValue( $this->getTalk( $target->getNamespace() ), $target->getDbKey() );
+
+               // NOTE: getTalk throws on bad namespaces!
+               return new TitleValue( $this->getTalk( $target->getNamespace() ), $target->getDBkey() );
+       }
+
+       /**
+        * Can the title have a corresponding talk page?
+        *
+        * False for relative section-only links (with getText() === ''),
+        * interwiki links (with getInterwiki() !== ''), and pages in NS_SPECIAL.
+        *
+        * @see getTalkPage
+        *
+        * @param LinkTarget $target
+        * @return bool True if this title either is a talk page or can have a talk page associated.
+        */
+       public function canHaveTalkPage( LinkTarget $target ) {
+               if ( $target->getText() === '' || $target->getInterwiki() !== '' ) {
+                       return false;
+               }
+
+               if ( $target->getNamespace() < NS_MAIN ) {
+                       return false;
+               }
+
+               return true;
        }
 
        /**
@@ -188,7 +227,7 @@ class NamespaceInfo {
                if ( $this->isSubject( $target->getNamespace() ) ) {
                        return $target;
                }
-               return new TitleValue( $this->getSubject( $target->getNamespace() ), $target->getDbKey() );
+               return new TitleValue( $this->getSubject( $target->getNamespace() ), $target->getDBkey() );
        }
 
        /**
@@ -216,8 +255,16 @@ class NamespaceInfo {
         * @throws MWException if $target's namespace doesn't have talk pages (e.g., NS_SPECIAL)
         */
        public function getAssociatedPage( LinkTarget $target ) : LinkTarget {
+               if ( $target->getText() === '' ) {
+                       throw new MWException( 'Can\'t determine talk page associated with relative section link' );
+               }
+
+               if ( $target->getInterwiki() !== '' ) {
+                       throw new MWException( 'Can\'t determine talk page associated with interwiki link' );
+               }
+
                return new TitleValue(
-                       $this->getAssociated( $target->getNamespace() ), $target->getDbKey() );
+                       $this->getAssociated( $target->getNamespace() ), $target->getDBkey() );
        }
 
        /**
index 9b021c4..4ffef02 100644 (file)
@@ -800,6 +800,65 @@ class TitleTest extends MediaWikiTestCase {
                        'Virtual namespace cannot have talk page' => [
                                Title::makeTitle( NS_MEDIA, 'Kitten.jpg' ), false
                        ],
+                       'Relative link has no talk page' => [
+                               Title::makeTitle( NS_MAIN, '', 'Kittens' ), false
+                       ],
+                       'Interwiki link has no talk page' => [
+                               Title::makeTitle( NS_MAIN, 'Kittens', '', 'acme' ), false
+                       ],
+               ];
+       }
+
+       public function provideIsWatchable() {
+               return [
+                       'User page is watchable' => [
+                               Title::makeTitle( NS_USER, 'Jane' ), true
+                       ],
+                       'Talke page is watchable' => [
+                               Title::makeTitle( NS_TALK, 'Foo' ), true
+                       ],
+                       'Special page is not watchable' => [
+                               Title::makeTitle( NS_SPECIAL, 'Thing' ), false
+                       ],
+                       'Virtual namespace is not watchable' => [
+                               Title::makeTitle( NS_MEDIA, 'Kitten.jpg' ), false
+                       ],
+                       'Relative link is not watchable' => [
+                               Title::makeTitle( NS_MAIN, '', 'Kittens' ), false
+                       ],
+                       'Interwiki link is not watchable' => [
+                               Title::makeTitle( NS_MAIN, 'Kittens', '', 'acme' ), false
+                       ],
+               ];
+       }
+
+       public static function provideGetTalkPage_good() {
+               return [
+                       [ Title::makeTitle( NS_MAIN, 'Test' ), Title::makeTitle( NS_TALK, 'Test' ) ],
+                       [ Title::makeTitle( NS_TALK, 'Test' ), Title::makeTitle( NS_TALK, 'Test' ) ],
+               ];
+       }
+
+       public static function provideGetTalkPage_bad() {
+               return [
+                       [ Title::makeTitle( NS_SPECIAL, 'Test' ) ],
+                       [ Title::makeTitle( NS_MEDIA, 'Test' ) ],
+                       [ Title::makeTitle( NS_MAIN, '', 'Kittens' ) ],
+                       [ Title::makeTitle( NS_MAIN, 'Kittens', '', 'acme' ) ],
+               ];
+       }
+
+       public static function provideGetSubjectPage_good() {
+               return [
+                       [ Title::makeTitle( NS_TALK, 'Test' ), Title::makeTitle( NS_MAIN, 'Test' ) ],
+                       [ Title::makeTitle( NS_MAIN, 'Test' ), Title::makeTitle( NS_MAIN, 'Test' ) ],
+               ];
+       }
+
+       public static function provideGetOtherPage_good() {
+               return [
+                       [ Title::makeTitle( NS_MAIN, 'Test' ), Title::makeTitle( NS_TALK, 'Test' ) ],
+                       [ Title::makeTitle( NS_TALK, 'Test' ), Title::makeTitle( NS_MAIN, 'Test' ) ],
                ];
        }
 
@@ -815,31 +874,44 @@ class TitleTest extends MediaWikiTestCase {
                $this->assertSame( $expected, $actual, $title->getPrefixedDBkey() );
        }
 
-       public static function provideGetTalkPage_good() {
-               return [
-                       [ Title::makeTitle( NS_MAIN, 'Test' ), Title::makeTitle( NS_TALK, 'Test' ) ],
-                       [ Title::makeTitle( NS_TALK, 'Test' ), Title::makeTitle( NS_TALK, 'Test' ) ],
-               ];
+       /**
+        * @dataProvider provideIsWatchable
+        * @covers Title::isWatchable
+        *
+        * @param Title $title
+        * @param bool $expected
+        */
+       public function testIsWatchable( Title $title, $expected ) {
+               $actual = $title->canHaveTalkPage();
+               $this->assertSame( $expected, $actual, $title->getPrefixedDBkey() );
        }
 
        /**
         * @dataProvider provideGetTalkPage_good
         * @covers Title::getTalkPageIfDefined
         */
-       public function testGetTalkPageIfDefined_good( Title $title ) {
-               $talk = $title->getTalkPageIfDefined();
-               $this->assertInstanceOf(
-                       Title::class,
-                       $talk,
-                       $title->getPrefixedDBKey()
-               );
+       public function testGetTalkPage_good( Title $title, Title $expected ) {
+               $actual = $title->getTalkPage();
+               $this->assertTrue( $expected->equals( $actual ), $title->getPrefixedDBkey() );
        }
 
-       public static function provideGetTalkPage_bad() {
-               return [
-                       [ Title::makeTitle( NS_SPECIAL, 'Test' ) ],
-                       [ Title::makeTitle( NS_MEDIA, 'Test' ) ],
-               ];
+       /**
+        * @dataProvider provideGetTalkPage_bad
+        * @covers Title::getTalkPageIfDefined
+        */
+       public function testGetTalkPage_bad( Title $title ) {
+               $this->setExpectedException( MWException::class );
+               $title->getTalkPage();
+       }
+
+       /**
+        * @dataProvider provideGetTalkPage_good
+        * @covers Title::getTalkPageIfDefined
+        */
+       public function testGetTalkPageIfDefined_good( Title $title, Title $expected ) {
+               $actual = $title->getTalkPageIfDefined();
+               $this->assertNotNull( $actual, $title->getPrefixedDBkey() );
+               $this->assertTrue( $expected->equals( $actual ), $title->getPrefixedDBkey() );
        }
 
        /**
@@ -850,10 +922,37 @@ class TitleTest extends MediaWikiTestCase {
                $talk = $title->getTalkPageIfDefined();
                $this->assertNull(
                        $talk,
-                       $title->getPrefixedDBKey()
+                       $title->getPrefixedDBkey()
                );
        }
 
+       /**
+        * @dataProvider provideGetSubjectPage_good
+        * @covers Title::getSubjectPage
+        */
+       public function testGetSubjectPage_good( Title $title, Title $expected ) {
+               $actual = $title->getSubjectPage();
+               $this->assertTrue( $expected->equals( $actual ), $title->getPrefixedDBkey() );
+       }
+
+       /**
+        * @dataProvider provideGetOtherPage_good
+        * @covers Title::getOtherPage
+        */
+       public function testGetOtherPage_good( Title $title, Title $expected ) {
+               $actual = $title->getOtherPage();
+               $this->assertTrue( $expected->equals( $actual ), $title->getPrefixedDBkey() );
+       }
+
+       /**
+        * @dataProvider provideGetTalkPage_bad
+        * @covers Title::getOtherPage
+        */
+       public function testGetOtherPage_bad( Title $title ) {
+               $this->setExpectedException( MWException::class );
+               $title->getOtherPage();
+       }
+
        public function provideCreateFragmentTitle() {
                return [
                        [ Title::makeTitle( NS_MAIN, 'Test' ), 'foo' ],
index c630447..ef2f219 100644 (file)
@@ -1,9 +1,11 @@
 <?php
+use MediaWiki\MediaWikiServices;
+
 /**
  * @group Database
  * @covers CoreParserFunctions
  */
-class CoreParserFunctionsTest extends MediaWikiTestCase {
+class CoreParserFunctionsTest extends MediaWikiLangTestCase {
 
        public function testGender() {
                $user = User::createNew( '*Female' );
@@ -18,4 +20,56 @@ class CoreParserFunctionsTest extends MediaWikiTestCase {
                $this->assertEquals( $msg, 'f', 'Works escaped' );
        }
 
+       public function provideTalkpagename() {
+               yield [ 'Talk:Foo bar', 'foo_bar' ];
+               yield [ 'Talk:Foo', ' foo ' ];
+               yield [ 'Talk:Foo', 'Talk:Foo' ];
+               yield [ 'User talk:Foo', 'User:foo' ];
+               yield [ '', 'Special:Foo' ];
+               yield [ '', '' ];
+               yield [ '', ' ' ];
+               yield [ '', '__' ];
+               yield [ '', '#xyzzy' ];
+               yield [ '', '#' ];
+               yield [ '', ':' ];
+               yield [ '', ':#' ];
+               yield [ '', 'User:' ];
+               yield [ '', 'User:#' ];
+       }
+
+       /**
+        * @dataProvider provideTalkpagename
+        */
+       public function testTalkpagename( $expected, $title ) {
+               $parser = MediaWikiServices::getInstance()->getParser();
+
+               $this->assertSame( $expected, CoreParserFunctions::talkpagename( $parser, $title ) );
+       }
+
+       public function provideSubjectpagename() {
+               yield [ 'Foo bar', 'Talk:foo_bar' ];
+               yield [ 'Foo', ' Talk:foo ' ];
+               yield [ 'User:Foo', 'User talk:foo' ];
+               yield [ 'Special:Foo', 'Special:Foo' ];
+               yield [ '', '' ];
+               yield [ '', ' ' ];
+               yield [ '', '__' ];
+               yield [ '', '#xyzzy' ];
+               yield [ '', '#' ];
+               yield [ '', ':' ];
+               yield [ '', ':#' ];
+               yield [ '', 'Talk:' ];
+               yield [ '', 'User talk:#' ];
+               yield [ '', 'User:#' ];
+       }
+
+       /**
+        * @dataProvider provideTalkpagename
+        */
+       public function testSubjectpagename( $expected, $title ) {
+               $parser = MediaWikiServices::getInstance()->getParser();
+
+               $this->assertSame( $expected, CoreParserFunctions::talkpagename( $parser, $title ) );
+       }
+
 }
index 7eb8fd5..c1e258d 100644 (file)
@@ -6,6 +6,7 @@
  */
 
 use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Linker\LinkTarget;
 
 class NamespaceInfoTest extends MediaWikiTestCase {
        /**********************************************************************************************
@@ -688,73 +689,88 @@ class NamespaceInfoTest extends MediaWikiTestCase {
 
        /**
         * @dataProvider provideSpecialNamespaces
-        * @covers NamespaceInfo::getTalk
-        * @covers NamespaceInfo::getTalkPage
+        * @covers NamespaceInfo::getAssociated
         * @covers NamespaceInfo::isMethodValidFor
         *
         * @param int $ns
         */
-       public function testGetTalkPage_special( $ns ) {
-               $this->setExpectedException( MWException::class,
-                       "NamespaceInfo::getTalk does not make any sense for given namespace $ns" );
-               $this->newObj()->getTalkPage( new TitleValue( $ns, 'A' ) );
+       public function testGetAssociated_special( $ns ) {
+               $this->setExpectedException(
+                       MWException::class,
+                       "NamespaceInfo::getAssociated does not make any sense for given namespace $ns"
+               );
+               $this->newObj()->getAssociated( $ns );
+       }
+
+       public static function provideCanHaveTalkPage() {
+               return [
+                       [ new TitleValue( NS_MAIN, 'Test' ), true ],
+                       [ new TitleValue( NS_TALK, 'Test' ), true ],
+                       [ new TitleValue( NS_USER, 'Test' ), true ],
+                       [ new TitleValue( NS_SPECIAL, 'Test' ), false ],
+                       [ new TitleValue( NS_MEDIA, 'Test' ), false ],
+                       [ new TitleValue( NS_MAIN, '', 'Kittens' ), false ],
+                       [ new TitleValue( NS_MAIN, 'Kittens', '', 'acme' ), false ],
+               ];
        }
 
        /**
-        * @dataProvider provideSpecialNamespaces
-        * @covers NamespaceInfo::getTalk
-        * @covers NamespaceInfo::getTalkPage
-        * @covers NamespaceInfo::isMethodValidFor
-        * @covers Title::getTalkPage
-        *
-        * @param int $ns
+        * @dataProvider provideCanHaveTalkPage
+        * @covers NamespaceInfo::canHaveTalkPage
         */
-       public function testTitleGetTalkPage_special( $ns ) {
-               $this->setExpectedException( MWException::class,
-                       "NamespaceInfo::getTalk does not make any sense for given namespace $ns" );
-               Title::makeTitle( $ns, 'A' )->getTalkPage();
+       public function testCanHaveTalkPage( LinkTarget $t, $expected ) {
+               $actual = $this->newObj()->canHaveTalkPage( $t );
+               $this->assertEquals( $expected, $actual, $t->getDBkey() );
+       }
+
+       public static function provideGetTalkPage_good() {
+               return [
+                       [ new TitleValue( NS_MAIN, 'Test' ), new TitleValue( NS_TALK, 'Test' ) ],
+                       [ new TitleValue( NS_TALK, 'Test' ), new TitleValue( NS_TALK, 'Test' ) ],
+                       [ new TitleValue( NS_USER, 'Test' ), new TitleValue( NS_USER_TALK, 'Test' ) ],
+               ];
        }
 
        /**
-        * @dataProvider provideSpecialNamespaces
-        * @covers NamespaceInfo::getAssociated
+        * @dataProvider provideGetTalkPage_good
+        * @covers NamespaceInfo::getTalk
+        * @covers NamespaceInfo::getTalkPage
         * @covers NamespaceInfo::isMethodValidFor
-        *
-        * @param int $ns
         */
-       public function testGetAssociated_special( $ns ) {
-               $this->setExpectedException( MWException::class,
-                       "NamespaceInfo::getAssociated does not make any sense for given namespace $ns" );
-               $this->newObj()->getAssociated( $ns );
+       public function testGetTalkPage_good( LinkTarget $t, LinkTarget $expected ) {
+               $actual = $this->newObj()->getTalkPage( $t );
+               $this->assertEquals( $expected, $actual, $t->getDBkey() );
+       }
+
+       public static function provideGetTalkPage_bad() {
+               return [
+                       [ new TitleValue( NS_SPECIAL, 'Test' ) ],
+                       [ new TitleValue( NS_MEDIA, 'Test' ) ],
+                       [ new TitleValue( NS_MAIN, '', 'Kittens' ) ],
+                       [ new TitleValue( NS_MAIN, 'Kittens', '', 'acme' ) ],
+               ];
        }
 
        /**
-        * @dataProvider provideSpecialNamespaces
-        * @covers NamespaceInfo::getAssociated
-        * @covers NamespaceInfo::getAssociatedPage
+        * @dataProvider provideGetTalkPage_bad
+        * @covers NamespaceInfo::getTalk
+        * @covers NamespaceInfo::getTalkPage
         * @covers NamespaceInfo::isMethodValidFor
-        *
-        * @param int $ns
         */
-       public function testGetAssociatedPage_special( $ns ) {
-               $this->setExpectedException( MWException::class,
-                       "NamespaceInfo::getAssociated does not make any sense for given namespace $ns" );
-               $this->newObj()->getAssociatedPage( new TitleValue( $ns, 'A' ) );
+       public function testGetTalkPage_bad( LinkTarget $t ) {
+               $this->setExpectedException( MWException::class );
+               $this->newObj()->getTalkPage( $t );
        }
 
        /**
-        * @dataProvider provideSpecialNamespaces
+        * @dataProvider provideGetTalkPage_bad
         * @covers NamespaceInfo::getAssociated
         * @covers NamespaceInfo::getAssociatedPage
         * @covers NamespaceInfo::isMethodValidFor
-        * @covers Title::getOtherPage
-        *
-        * @param int $ns
         */
-       public function testTitleGetOtherPage_special( $ns ) {
-               $this->setExpectedException( MWException::class,
-                       "NamespaceInfo::getAssociated does not make any sense for given namespace $ns" );
-               Title::makeTitle( $ns, 'A' )->getOtherPage();
+       public function testGetAssociatedPage_bad( LinkTarget $t ) {
+               $this->setExpectedException( MWException::class );
+               $this->newObj()->getAssociatedPage( $t );
        }
 
        /**