Merge "Don't fail hard on bad titles in the database."

includes/Linker.php

@@ -914,7 +914,7 @@ class Linker {
 			}
 			$classes .= ' mw-anonuserlink'; // Separate link class for anons (T45179)
 		} else {
-			$page = new TitleValue( NS_USER, strtr( $userName, ' ', '_' ) );
+			$page = TitleValue::tryNew( NS_USER, strtr( $userName, ' ', '_' ) );
 		}
 
 		// Wrap the output with <bdi> tags for directionality isolation
@@ -1040,13 +1040,13 @@ class Linker {
 			return wfMessage( 'empty-username' )->parse();
 		}
 
-		$userTalkPage = new TitleValue( NS_USER_TALK, strtr( $userText, ' ', '_' ) );
+		$userTalkPage = TitleValue::tryNew( NS_USER_TALK, strtr( $userText, ' ', '_' ) );
 		$moreLinkAttribs = [ 'class' => 'mw-usertoollinks-talk' ];
+		$linkText = wfMessage( 'talkpagelinktext' )->escaped();
 
-		return self::link( $userTalkPage,
-			wfMessage( 'talkpagelinktext' )->escaped(),
-			$moreLinkAttribs
-		);
+		return $userTalkPage
+			? self::link( $userTalkPage, $linkText, $moreLinkAttribs )
+			: Html::rawElement( 'span', $moreLinkAttribs, $linkText );
 	}
 
 	/**

includes/api/ApiQueryBase.php

@@ -522,21 +522,42 @@ abstract class ApiQueryBase extends ApiBase {
 	}
 
 	/**
-	 * Convert an input title or title prefix into a namespace constant and dbkey.
+	 * Convert an input title or title prefix into a TitleValue.
 	 *
-	 * @since 1.26
+	 * @since 1.35
 	 * @param string $titlePart Title part
 	 * @param int $defaultNamespace Default namespace if none is given
-	 * @return array (int, string) Namespace number and DBkey
+	 * @return TitleValue
 	 */
-	public function prefixedTitlePartToKey( $titlePart, $defaultNamespace = NS_MAIN ) {
-		$t = Title::newFromText( $titlePart . 'x', $defaultNamespace );
-		if ( !$t || $t->hasFragment() || $t->isExternal() ) {
+	protected function parsePrefixedTitlePart( $titlePart, $defaultNamespace = NS_MAIN ) {
+		try {
+			$titleParser = MediaWikiServices::getInstance()->getTitleParser();
+			$t = $titleParser->parseTitle( $titlePart . 'X', $defaultNamespace );
+		} catch ( MalformedTitleException $e ) {
+			$t = null;
+		}
+
+		if ( !$t || $t->hasFragment() || $t->isExternal() || $t->getDBkey() === 'X' ) {
 			// Invalid title (e.g. bad chars) or contained a '#'.
 			$this->dieWithError( [ 'apierror-invalidtitle', wfEscapeWikiText( $titlePart ) ] );
 		}
 
-		return [ $t->getNamespace(), substr( $t->getDBkey(), 0, -1 ) ];
+		return new TitleValue( $t->getNamespace(), substr( $t->getDBkey(), 0, -1 ) );
+	}
+
+	/**
+	 * Convert an input title or title prefix into a namespace constant and dbkey.
+	 *
+	 * @since 1.26
+	 * @deprecated sine 1.35, use parsePrefixedTitlePart() instead.
+	 * @param string $titlePart Title part parsePrefixedTitlePart instead
+	 * @param int $defaultNamespace Default namespace if none is given
+	 * @return array (int, string) Namespace number and DBkey
+	 */
+	public function prefixedTitlePartToKey( $titlePart, $defaultNamespace = NS_MAIN ) {
+		wfDeprecated( __METHOD__, '1.35' );
+		$t = $this->parsePrefixedTitlePart( $titlePart, $defaultNamespace );
+		return [ $t->getNamespace(), $t->getDBkey() ];
 	}
 
 	/**

includes/api/ApiQueryWatchlistRaw.php

@@ -76,17 +76,16 @@ class ApiQueryWatchlistRaw extends ApiQueryGeneratorBase {
 			$ns = (int)$cont[0];
 			$this->dieContinueUsageIf( strval( $ns ) !== $cont[0] );
 			$title = $cont[1];
-			$options['startFrom'] = new TitleValue( $ns, $title );
+			$options['startFrom'] = TitleValue::tryNew( $ns, $title );
+			$this->dieContinueUsageIf( !$options['startFrom'] );
 		}
 
 		if ( isset( $params['fromtitle'] ) ) {
-			list( $ns, $title ) = $this->prefixedTitlePartToKey( $params['fromtitle'] );
-			$options['from'] = new TitleValue( $ns, $title );
+			$options['from'] = $this->parsePrefixedTitlePart( $params['fromtitle'] );
 		}
 
 		if ( isset( $params['totitle'] ) ) {
-			list( $ns, $title ) = $this->prefixedTitlePartToKey( $params['totitle'] );
-			$options['until'] = new TitleValue( $ns, $title );
+			$options['until'] = $this->parsePrefixedTitlePart( $params['totitle'] );
 		}
 
 		$options['sort'] = WatchedItemStore::SORT_ASC;

includes/cache/LinkBatch.php

@@ -166,19 +166,29 @@ class LinkBatch {
 		$ids = [];
 		$remaining = $this->data;
 		foreach ( $res as $row ) {
-			$title = new TitleValue( (int)$row->page_namespace, $row->page_title );
-			$cache->addGoodLinkObjFromRow( $title, $row );
-			$pdbk = $titleFormatter->getPrefixedDBkey( $title );
-			$ids[$pdbk] = $row->page_id;
+			$title = TitleValue::tryNew( (int)$row->page_namespace, $row->page_title );
+			if ( $title ) {
+				$cache->addGoodLinkObjFromRow( $title, $row );
+				$pdbk = $titleFormatter->getPrefixedDBkey( $title );
+				$ids[$pdbk] = $row->page_id;
+			} else {
+				wfLogWarning( __METHOD__ . ': encountered invalid title: ' . $row->page_title );
+			}
+
 			unset( $remaining[$row->page_namespace][$row->page_title] );
 		}
 
 		// The remaining links in $data are bad links, register them as such
 		foreach ( $remaining as $ns => $dbkeys ) {
 			foreach ( $dbkeys as $dbkey => $unused ) {
-				$title = new TitleValue( (int)$ns, (string)$dbkey );
-				$cache->addBadLinkObj( $title );
-				$pdbk = $titleFormatter->getPrefixedDBkey( $title );
+				$title = TitleValue::tryNew( (int)$ns, (string)$dbkey );
+				if ( $title ) {
+					$cache->addBadLinkObj( $title );
+					$pdbk = $titleFormatter->getPrefixedDBkey( $title );
+				} else {
+					wfLogWarning( __METHOD__ . ': encountered invalid title: ' . $row->page_title );
+				}
+
 				$ids[$pdbk] = 0;
 			}
 		}

includes/specials/SpecialEditWatchlist.php

@@ -446,7 +446,10 @@ class SpecialEditWatchlist extends UnlistedSpecialPage {
 				wfDebug( "User {$user->getName()} has broken watchlist item " .
 					"ns($namespace):$dbKey, $action.\n" );
 
-				$store->removeWatch( $user, new TitleValue( (int)$namespace, $dbKey ) );
+				// NOTE: We *know* that the title is invalid. TitleValue may refuse instantiation.
+				// XXX: We may need an InvalidTitleValue class that allows instantiation of
+				//      known bad title values.
+				$store->removeWatch( $user, Title::makeTitle( (int)$namespace, $dbKey ) );
 				// Can't just do an UPDATE instead of DELETE/INSERT due to unique index
 				if ( $title ) {
 					$user->addWatch( $title );

includes/title/TitleValue.php

@@ -22,6 +22,7 @@
  */
 use MediaWiki\Linker\LinkTarget;
 use Wikimedia\Assert\Assert;
+use Wikimedia\Assert\ParameterAssertionException;
 use Wikimedia\Assert\ParameterTypeException;
 
 /**
@@ -69,6 +70,36 @@ class TitleValue implements LinkTarget {
 	 */
 	public $prefixedText = null;
 
+	/**
+	 * Constructs a TitleValue, or returns null if the parameters are not valid.
+	 *
+	 * @note This does not perform any normalization, and only basic validation.
+	 * For full normalization and validation, use TitleParser::makeTitleValueSafe().
+	 *
+	 * @param int $namespace The namespace ID. This is not validated.
+	 * @param string $title The page title in either DBkey or text form. No normalization is applied
+	 *   beyond underscore/space conversion.
+	 * @param string $fragment The fragment title. Use '' to represent the whole page.
+	 *   No validation or normalization is applied.
+	 * @param string $interwiki The interwiki component.
+	 *   No validation or normalization is applied.
+	 *
+	 * @return TitleValue|null
+	 *
+	 * @throws InvalidArgumentException
+	 */
+	public static function tryNew( $namespace, $title, $fragment = '', $interwiki = '' ) {
+		if ( !is_int( $namespace ) ) {
+			throw new ParameterTypeException( '$namespace', 'int' );
+		}
+
+		try {
+			return new static( $namespace, $title, $fragment, $interwiki );
+		} catch ( ParameterAssertionException $ex ) {
+			return null;
+		}
+	}
+
 	/**
 	 * Constructs a TitleValue.
 	 *
@@ -81,7 +112,8 @@ class TitleValue implements LinkTarget {
 	 *   beyond underscore/space conversion.
 	 * @param string $fragment The fragment title. Use '' to represent the whole page.
 	 *   No validation or normalization is applied.
-	 * @param string $interwiki The interwiki component
+	 * @param string $interwiki The interwiki component.
+	 *   No validation or normalization is applied.
 	 *
 	 * @throws InvalidArgumentException
 	 */

tests/phpunit/includes/cache/LinkBatchTest.php

@@ -0,0 +1,119 @@
+<?php
+
+use Wikimedia\AtEase\AtEase;
+
+/**
+ * @group Database
+ * @group Cache
+ * @covers LinkBatch
+ */
+class LinkBatchTest extends MediaWikiIntegrationTestCase {
+
+	/**
+	 * @covers LinkBatch::__construct()
+	 * @covers LinkBatch::getSize()
+	 * @covers LinkBatch::isEmpty()
+	 */
+	public function testConstructEmpty() {
+		$batch = new LinkBatch();
+
+		$this->assertTrue( $batch->isEmpty() );
+		$this->assertSame( 0, $batch->getSize() );
+	}
+
+	/**
+	 * @covers LinkBatch::__construct()
+	 * @covers LinkBatch::getSize()
+	 * @covers LinkBatch::isEmpty()
+	 */
+	public function testConstruct() {
+		$batch = new LinkBatch( [
+			new TitleValue( NS_MAIN, 'Foo' ),
+			new TitleValue( NS_TALK, 'Bar' ),
+		] );
+
+		$this->assertFalse( $batch->isEmpty() );
+		$this->assertSame( 2, $batch->getSize() );
+	}
+
+	/**
+	 * @covers LinkBatch::addObj()
+	 * @covers LinkBatch::getSize()
+	 */
+	public function testAddObj() {
+		$batch = new LinkBatch( [
+			new TitleValue( NS_MAIN, 'Foo' )
+		] );
+
+		$batch->addObj( new TitleValue( NS_TALK, 'Bar' ) );
+		$batch->addObj( new TitleValue( NS_MAIN, 'Foo' ) );
+
+		$this->assertSame( 2, $batch->getSize() );
+	}
+
+	/**
+	 * @covers LinkBatch::add()
+	 * @covers LinkBatch::getSize()
+	 */
+	public function testAdd() {
+		$batch = new LinkBatch( [
+			new TitleValue( NS_MAIN, 'Foo' )
+		] );
+
+		$batch->add( NS_TALK, 'Bar' );
+		$batch->add( NS_MAIN, 'Foo' );
+
+		$this->assertSame( 2, $batch->getSize() );
+	}
+
+	public function testExecute() {
+		$existing1 = $this->getExistingTestPage( __METHOD__ . '1' )->getTitle();
+		$existing2 = $this->getExistingTestPage( __METHOD__ . '2' )->getTitle();
+		$nonexisting1 = $this->getNonexistingTestPage( __METHOD__ . 'x' )->getTitle();
+		$nonexisting2 = $this->getNonexistingTestPage( __METHOD__ . 'y' )->getTitle();
+
+		$cache = $this->getMockBuilder( LinkCache::class )
+			->disableOriginalConstructor()
+			->getMock();
+
+		$good = [];
+		$bad = [];
+
+		$cache->expects( $this->exactly( 2 ) )
+			->method( 'addGoodLinkObjFromRow' )
+			->willReturnCallback( function ( TitleValue $title, $row ) use ( &$good ) {
+				$good["$title"] = $title;
+			} );
+
+		$cache->expects( $this->exactly( 2 ) )
+			->method( 'addBadLinkObj' )
+			->willReturnCallback( function ( TitleValue $title ) use ( &$bad ) {
+				$bad["$title"] = $title;
+			} );
+
+		$this->setService( 'LinkCache', $cache );
+
+		$batch = new LinkBatch();
+
+		$batch->addObj( $existing1 );
+		$batch->addObj( $existing2 );
+		$batch->addObj( $nonexisting1 );
+		$batch->addObj( $nonexisting2 );
+
+		// Bad stuff, should be skipped!
+		$batch->add( NS_MAIN, '_X' );
+		$batch->add( NS_MAIN, 'X_' );
+		$batch->add( NS_MAIN, '' );
+
+		AtEase::suppressWarnings();
+		$batch->execute();
+		AtEase::restoreWarnings();
+
+		$this->assertArrayHasKey( $existing1->getTitleValue()->__toString(), $good );
+		$this->assertArrayHasKey( $existing2->getTitleValue()->__toString(), $good );
+
+		$this->assertArrayHasKey( $nonexisting1->getTitleValue()->__toString(), $bad );
+		$this->assertArrayHasKey( $nonexisting2->getTitleValue()->__toString(), $bad );
+	}
+
+}

tests/phpunit/includes/title/MediaWikiTitleCodecTest.php

@@ -503,6 +503,8 @@ class MediaWikiTitleCodecTest extends MediaWikiTestCase {
 	public static function provideMakeTitleValueSafe() {
 		$ret = [
 			'Nonexistent NS' => [ null, 942929, 'Test' ],
+			'Linebreak in title' => [ null, NS_MAIN, "Test\nthis" ],
+			'Pipe in title' => [ null, NS_MAIN, "Test|this" ],
 			'Simple page' => [ new TitleValue( NS_MAIN, 'Test' ), NS_MAIN, 'Test' ],
 
 			// Fragments

tests/phpunit/unit/includes/title/TitleValueTest.php

@@ -57,6 +57,24 @@ class TitleValueTest extends \MediaWikiUnitTestCase {
 		$this->assertEquals( $hasInterwiki, $title->isExternal() );
 	}
 
+	/**
+	 * @dataProvider goodConstructorProvider
+	 */
+	public function testTryNew( $ns, $text, $fragment, $interwiki, $hasFragment,
+		$hasInterwiki
+	) {
+		$title = TitleValue::tryNew( $ns, $text, $fragment, $interwiki );
+
+		$this->assertEquals( $ns, $title->getNamespace() );
+		$this->assertTrue( $title->inNamespace( $ns ) );
+		$this->assertEquals( strtr( $text, ' ', '_' ), $title->getDbKey() );
+		$this->assertEquals( strtr( $text, '_', ' ' ), $title->getText() );
+		$this->assertEquals( $fragment, $title->getFragment() );
+		$this->assertEquals( $hasFragment, $title->hasFragment() );
+		$this->assertEquals( $interwiki, $title->getInterwiki() );
+		$this->assertEquals( $hasInterwiki, $title->isExternal() );
+	}
+
 	/**
 	 * @dataProvider goodConstructorProvider
 	 */
@@ -65,12 +83,16 @@ class TitleValueTest extends \MediaWikiUnitTestCase {
 		$this->assertTrue( true ); // we are just checking that no exception is thrown
 	}
 
-	public function badConstructorProvider() {
+	public function badConstructorNamespaceTypeProvider() {
 		return [
 			[ 'foo', 'title', 'fragment', '' ],
 			[ null, 'title', 'fragment', '' ],
 			[ 2.3, 'title', 'fragment', '' ],
+		];
+	}
 
+	public function badConstructorProvider() {
+		return [
 			[ NS_MAIN, 5, 'fragment', '' ],
 			[ NS_MAIN, null, 'fragment', '' ],
 			[ NS_USER, '', 'fragment', '' ],
@@ -90,6 +112,7 @@ class TitleValueTest extends \MediaWikiUnitTestCase {
 	}
 
 	/**
+	 * @dataProvider badConstructorNamespaceTypeProvider
 	 * @dataProvider badConstructorProvider
 	 */
 	public function testConstructionErrors( $ns, $text, $fragment, $interwiki ) {
@@ -97,6 +120,21 @@ class TitleValueTest extends \MediaWikiUnitTestCase {
 		new TitleValue( $ns, $text, $fragment, $interwiki );
 	}
 
+	/**
+	 * @dataProvider badConstructorNamespaceTypeProvider
+	 */
+	public function testTryNewErrors( $ns, $text, $fragment, $interwiki ) {
+		$this->expectException( InvalidArgumentException::class );
+		TitleValue::tryNew( $ns, $text, $fragment, $interwiki );
+	}
+
+	/**
+	 * @dataProvider badConstructorProvider
+	 */
+	public function testTryNewFailure( $ns, $text, $fragment, $interwiki ) {
+		$this->assertNull( TitleValue::tryNew( $ns, $text, $fragment, $interwiki ) );
+	}
+
 	/**
 	 * @dataProvider badConstructorProvider
 	 */