Move ::addTrackingCategory() implementation to TrackingCategories

This moves the implementation of ParserOutput::addTrackingCategory()
to the TrackingCategories class as a non-static method. This makes
invocation from ParserOutput awkward, but when invoking as
Parser::addTrackingCategory() all the necessary services are
available.  As a result, we've also soft-deprecated
ParserOutput::addTrackingCategory(); new users should use the
TrackingCategories::addTrackingCategory() method, or else
Parser::addTrackingCategory() if the parser object is available.

The Parser class is already kind of bloated as it is (alas), but there
aren't too many callsites which invoke
ParserOutput::addTrackingCategory() and don't have the corresponding
Parser object handy; see:

https://codesearch.wmcloud.org/search/?q=%5BOo%5Dutput%28%5C%28%5C%29%29%3F-%3EaddTrackingCategory%5C%28&i=nope&files=&excludeFiles=&repos=

Change-Id: I697ce188a912e445a6a748121575548e79aabac6

RELEASE-NOTES-1.38

@@ -223,7 +223,11 @@ because of Phabricator reports.
    - ::getProperties()       - use ::getPageProperties()
    - ::getCategoryLinks()    - use ::getCategoryNames()
    - ::setCategoryLinks()    - use ::setCategories()
-* The following methods were deprecated; use ::setPreventClickjacking() instead:
+* The following methods from the ParserOutput class were deprecated:
+   - ::addTrackingCategory() - use Parser::addTrackingCategory()
+                               or TrackingCategories::addTrackingCategory()
+* The following methods were deprecated; use ::setPreventClickjacking(..)
+  instead:
    - OutputPage::preventClickjacking()
    - OutputPage::allowClickjacking()
    - ImageHistoryList::preventClickjacking()

includes/ServiceWiring.php

@@ -1225,7 +1225,8 @@ return [
 			$services->getUserOptionsLookup(),
 			$services->getUserFactory(),
 			$services->getTitleFormatter(),
-			$services->getHttpRequestFactory()
+			$services->getHttpRequestFactory(),
+			$services->getTrackingCategories()
 		);
 	},
 
@@ -1722,7 +1723,8 @@ return [
 				$services->getMainConfig()
 			),
 			$services->getNamespaceInfo(),
-			$services->getTitleFactory()
+			$services->getTitleParser(),
+			LoggerFactory::getInstance( 'TrackingCategories' )
 		);
 	},
 

includes/TrackingCategories.php

@@ -20,6 +20,9 @@
  */
 
 use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Linker\LinkTarget;
+use MediaWiki\Page\PageReference;
+use Psr\Log\LoggerInterface;
 
 /**
  * This class performs some operations related to tracking categories, such as creating
@@ -42,12 +45,15 @@ class TrackingCategories {
 	/** @var NamespaceInfo */
 	private $namespaceInfo;
 
-	/** @var TitleFactory */
+	/** @var TitleParser */
-	private $titleFactory;
+	private $titleParser;
 
 	/** @var ExtensionRegistry */
 	private $extensionRegistry;
 
+	/** @var LoggerInterface */
+	private $logger;
+
 	/**
 	 * Tracking categories that exist in core
 	 *
@@ -73,17 +79,20 @@ class TrackingCategories {
 	/**
 	 * @param ServiceOptions $options
 	 * @param NamespaceInfo $namespaceInfo
-	 * @param TitleFactory $titleFactory
+	 * @param TitleParser $titleParser
+	 * @param LoggerInterface $logger
 	 */
 	public function __construct(
 		ServiceOptions $options,
 		NamespaceInfo $namespaceInfo,
-		TitleFactory $titleFactory
+		TitleParser $titleParser,
+		LoggerInterface $logger
 	) {
 		$options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
 		$this->options = $options;
 		$this->namespaceInfo = $namespaceInfo;
-		$this->titleFactory = $titleFactory;
+		$this->titleParser = $titleParser;
+		$this->logger = $logger;
 
 		// TODO convert ExtensionRegistry to a service and inject it
 		$this->extensionRegistry = ExtensionRegistry::getInstance();
@@ -129,7 +138,7 @@ class TrackingCategories {
 			 */
 			$msgObj = wfMessage( $catMsg )->inContentLanguage();
 			$allCats = [];
-			$catMsgTitle = $this->titleFactory->makeTitleSafe( NS_MEDIAWIKI, $catMsg );
+			$catMsgTitle = $this->titleParser->makeTitleValueSafe( NS_MEDIAWIKI, $catMsg );
 			if ( !$catMsgTitle ) {
 				continue;
 			}
@@ -139,14 +148,17 @@ class TrackingCategories {
 			if ( strpos( $msgObj->plain(), '{{' ) !== false ) {
 				$ns = $this->namespaceInfo->getValidNamespaces();
 				foreach ( $ns as $namesp ) {
-					$tempTitle = $this->titleFactory->makeTitleSafe( $namesp, $catMsg );
+					$tempTitle = $this->titleParser->makeTitleValueSafe( $namesp, $catMsg );
 					if ( !$tempTitle ) {
 						continue;
 					}
+					// XXX: should be a better way to convert a TitleValue
+					// to a PageReference!
+					$tempTitle = Title::newFromTitleValue( $tempTitle );
 					$catName = $msgObj->page( $tempTitle )->text();
 					# Allow tracking categories to be disabled by setting them to "-"
 					if ( $catName !== '-' ) {
-						$catTitle = $this->titleFactory->makeTitleSafe( NS_CATEGORY, $catName );
+						$catTitle = $this->titleParser->makeTitleValueSafe( NS_CATEGORY, $catName );
 						if ( $catTitle ) {
 							$allCats[] = $catTitle;
 						}
@@ -156,7 +168,7 @@ class TrackingCategories {
 				$catName = $msgObj->text();
 				# Allow tracking categories to be disabled by setting them to "-"
 				if ( $catName !== '-' ) {
-					$catTitle = $this->titleFactory->makeTitleSafe( NS_CATEGORY, $catName );
+					$catTitle = $this->titleParser->makeTitleValueSafe( NS_CATEGORY, $catName );
 					if ( $catTitle ) {
 						$allCats[] = $catTitle;
 					}
@@ -170,4 +182,63 @@ class TrackingCategories {
 
 		return $trackingCategories;
 	}
+
+	/**
+	 * Resolve a tracking category.
+	 * @param string $msg Message key
+	 * @param ?PageReference $contextPage Context page title
+	 * @return ?LinkTarget the proper category page, or null if
+	 *   the tracking category is disabled or unsafe
+	 * @since 1.38
+	 */
+	public function resolveTrackingCategory( string $msg, ?PageReference $contextPage ): ?LinkTarget {
+		if ( !$contextPage ) {
+			$this->logger->debug( "Not adding tracking category $msg to missing page!" );
+			return null;
+		}
+
+		if ( $contextPage->getNamespace() === NS_SPECIAL ) {
+			$this->logger->debug( "Not adding tracking category $msg to special page!" );
+			return null;
+		}
+
+		// Important to parse with correct title (T33469)
+		// TODO replace uses of wfMessage with an injected service once that is available
+		$cat = wfMessage( $msg )
+			->page( $contextPage )
+			->inContentLanguage()
+			->text();
+
+		# Allow tracking categories to be disabled by setting them to "-"
+		if ( $cat === '-' ) {
+			return null;
+		}
+
+		$containerCategory = $this->titleParser->makeTitleValueSafe( NS_CATEGORY, $cat );
+		if ( $containerCategory === null ) {
+			$this->logger->debug( "[[MediaWiki:$msg]] is not a valid title!" );
+			return null;
+		}
+		return $containerCategory;
+	}
+
+	/**
+	 * Add a tracking category to a ParserOutput.
+	 * @param ParserOutput $parserOutput
+	 * @param string $msg Message key
+	 * @param ?PageReference $contextPage Context page title
+	 * @return bool Whether the addition was successful
+	 * @since 1.38
+	 */
+	public function addTrackingCategory( ParserOutput $parserOutput, string $msg, ?PageReference $contextPage ): bool {
+		$categoryPage = $this->resolveTrackingCategory( $msg, $contextPage );
+		if ( $categoryPage === null ) {
+			return false;
+		}
+		$parserOutput->addCategory(
+			$categoryPage->getDBkey(),
+			$parserOutput->getPageProperty( 'defaultsort' ) ?: ''
+		);
+		return true;
+	}
 }

includes/parser/Parser.php

@@ -363,6 +363,9 @@ class Parser {
 	/** @var HttpRequestFactory */
 	private $httpRequestFactory;
 
+	/** @var TrackingCategories */
+	private $trackingCategories;
+
 	/**
 	 * @internal For use by ServiceWiring
 	 */
@@ -410,6 +413,7 @@ class Parser {
 	 * @param UserFactory $userFactory
 	 * @param TitleFormatter $titleFormatter
 	 * @param HttpRequestFactory $httpRequestFactory
+	 * @param TrackingCategories $trackingCategories
 	 */
 	public function __construct(
 		ServiceOptions $svcOptions,
@@ -429,7 +433,8 @@ class Parser {
 		UserOptionsLookup $userOptionsLookup,
 		UserFactory $userFactory,
 		TitleFormatter $titleFormatter,
-		HttpRequestFactory $httpRequestFactory
+		HttpRequestFactory $httpRequestFactory,
+		TrackingCategories $trackingCategories
 	) {
 		if ( ParserFactory::$inParserFactory === 0 ) {
 			// Direct construction of Parser was deprecated in 1.34 and
@@ -475,6 +480,7 @@ class Parser {
 		$this->userFactory = $userFactory;
 		$this->titleFormatter = $titleFormatter;
 		$this->httpRequestFactory = $httpRequestFactory;
+		$this->trackingCategories = $trackingCategories;
 
 		// These steps used to be done in "::firstCallInit()"
 		// (if you're chasing a reference from some old code)
@@ -4085,7 +4091,9 @@ class Parser {
 	 * @since 1.19 method is public
 	 */
 	public function addTrackingCategory( $msg ) {
-		return $this->mOutput->addTrackingCategory( $msg, $this->getTitle() );
+		return $this->trackingCategories->addTrackingCategory(
+			$this->mOutput, $msg, $this->getPage()
+		);
 	}
 
 	/**

includes/parser/ParserFactory.php

@@ -77,6 +77,9 @@ class ParserFactory {
 	/** @var HttpRequestFactory */
 	private $httpRequestFactory;
 
+	/** @var TrackingCategories */
+	private $trackingCategories;
+
 	/**
 	 * Track calls to Parser constructor to aid in deprecation of direct
 	 * Parser invocation.  This is temporary: it will be removed once the
@@ -114,6 +117,7 @@ class ParserFactory {
 	 * @param UserFactory $userFactory
 	 * @param TitleFormatter $titleFormatter
 	 * @param HttpRequestFactory $httpRequestFactory
+	 * @param TrackingCategories $trackingCategories
 	 * @since 1.32
 	 * @internal
 	 */
@@ -134,7 +138,8 @@ class ParserFactory {
 		UserOptionsLookup $userOptionsLookup,
 		UserFactory $userFactory,
 		TitleFormatter $titleFormatter,
-		HttpRequestFactory $httpRequestFactory
+		HttpRequestFactory $httpRequestFactory,
+		TrackingCategories $trackingCategories
 	) {
 		$svcOptions->assertRequiredOptions( Parser::CONSTRUCTOR_OPTIONS );
 
@@ -157,6 +162,7 @@ class ParserFactory {
 		$this->userFactory = $userFactory;
 		$this->titleFormatter = $titleFormatter;
 		$this->httpRequestFactory = $httpRequestFactory;
+		$this->trackingCategories = $trackingCategories;
 	}
 
 	/**
@@ -186,7 +192,8 @@ class ParserFactory {
 				$this->userOptionsLookup,
 				$this->userFactory,
 				$this->titleFormatter,
-				$this->httpRequestFactory
+				$this->httpRequestFactory,
+				$this->trackingCategories
 			);
 		} finally {
 			self::$inParserFactory--;

includes/parser/ParserOutput.php

@@ -1074,33 +1074,12 @@ class ParserOutput extends CacheTime {
 	 *        (used to require a Title until 1.38)
 	 * @return bool Whether the addition was successful
 	 * @since 1.25
+	 * @deprecated since 1.38, use Parser::addTrackingCategory or
+	 *   TrackingCategories::addTrackingCategory() instead
 	 */
 	public function addTrackingCategory( $msg, PageReference $page ) {
-		if ( $page->getNamespace() === NS_SPECIAL ) {
+		$trackingCategories = MediaWikiServices::getInstance()->getTrackingCategories();
-			wfDebug( __METHOD__ . ": Not adding tracking category $msg to special page!" );
+		return $trackingCategories->addTrackingCategory( $this, $msg, $page );
-			return false;
-		}
-
-		// Important to parse with correct title (T33469)
-		$cat = wfMessage( $msg )
-			->page( $page )
-			->inContentLanguage()
-			->text();
-
-		// Allow tracking categories to be disabled by setting them to "-"
-		if ( $cat === '-' ) {
-			return false;
-		}
-
-		$titleParser = MediaWikiServices::getInstance()->getTitleParser();
-		$containerCategory = $titleParser->makeTitleValueSafe( NS_CATEGORY, $cat );
-		if ( $containerCategory ) {
-			$this->addCategory( $containerCategory->getDBkey(), $this->getPageProperty( 'defaultsort' ) ?: '' );
-			return true;
-		} else {
-			wfDebug( __METHOD__ . ": [[MediaWiki:$msg]] is not a valid title!" );
-			return false;
-		}
 	}
 
 	/**

tests/phpunit/includes/TrackingCategoriesTest.php

@@ -0,0 +1,47 @@
+<?php
+
+use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Logger\LoggerFactory;
+use MediaWiki\Page\PageReferenceValue;
+
+/**
+ * @covers ParserOutput
+ * @covers CacheTime
+ * @group Database
+ *        ^--- trigger DB shadowing because we are using Title magic
+ */
+class TrackingCategoriesTest extends MediaWikiLangTestCase {
+	/**
+	 * @covers ParserOutput::addTrackingCategory
+	 */
+	public function testAddTrackingCategory() {
+		$services = $this->getServiceContainer();
+		$tc = new TrackingCategories(
+			new ServiceOptions(
+				TrackingCategories::CONSTRUCTOR_OPTIONS,
+				$services->getMainConfig()
+			),
+			$services->getNamespaceInfo(),
+			$services->getTitleParser(),
+			LoggerFactory::getInstance( 'TrackingCategories' )
+		);
+
+		$po = new ParserOutput;
+		$po->setPageProperty( 'defaultsort', 'foobar' );
+
+		$page = PageReferenceValue::localReference( NS_USER, 'Testing' );
+
+		$tc->addTrackingCategory( $po, 'index-category', $page ); // from CORE_TRACKING_CATEGORIES
+		$tc->addTrackingCategory( $po, 'sitenotice', $page ); // should be "-", which is ignored
+		$tc->addTrackingCategory( $po, 'brackets-start', $page ); // invalid text
+		// TODO: assert proper handling of non-existing messages
+
+		$expected = wfMessage( 'index-category' )
+			->page( $page )
+			->inContentLanguage()
+			->text();
+
+		$expected = strtr( $expected, ' ', '_' );
+		$this->assertSame( [ $expected => 'foobar' ], $po->getCategories() );
+	}
+}

tests/phpunit/includes/parser/ParserTest.php

@@ -49,7 +49,8 @@ class ParserTest extends MediaWikiIntegrationTestCase {
 			$this->createMock( MediaWiki\User\UserOptionsLookup::class ),
 			$this->createMock( MediaWiki\User\UserFactory::class ),
 			$this->createMock( TitleFormatter::class ),
-			$this->createMock( HttpRequestFactory::class )
+			$this->createMock( HttpRequestFactory::class ),
+			$this->createMock( TrackingCategories::class )
 		];
 	}
 

tests/phpunit/unit/includes/parser/ParserFactoryTest.php

@@ -60,7 +60,8 @@ class ParserFactoryTest extends MediaWikiUnitTestCase {
 			$this->createNoOpMock( UserOptionsLookup::class ),
 			$this->createNoOpMock( UserFactory::class ),
 			$this->createNoOpMock( TitleFormatter::class ),
-			$this->createNoOpMock( HttpRequestFactory::class )
+			$this->createNoOpMock( HttpRequestFactory::class ),
+			$this->createNoOpMock( TrackingCategories::class )
 		);
 		return $factory;
 	}