From 9632dfb0413c8496ac7747bab92a7d286f8caef8 Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Fri, 8 Oct 2021 12:37:26 -0400 Subject: [PATCH] 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 | 6 +- includes/ServiceWiring.php | 6 +- includes/TrackingCategories.php | 89 +++++++++++++++++-- includes/parser/Parser.php | 12 ++- includes/parser/ParserFactory.php | 11 ++- includes/parser/ParserOutput.php | 29 +----- .../includes/TrackingCategoriesTest.php | 47 ++++++++++ tests/phpunit/includes/parser/ParserTest.php | 3 +- .../includes/parser/ParserFactoryTest.php | 3 +- 9 files changed, 163 insertions(+), 43 deletions(-) create mode 100644 tests/phpunit/includes/TrackingCategoriesTest.php diff --git a/RELEASE-NOTES-1.38 b/RELEASE-NOTES-1.38 index 3e16e592c4d..cb14323fadf 100644 --- a/RELEASE-NOTES-1.38 +++ b/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() diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 9289f3b292d..3750b7bd878 100644 --- a/includes/ServiceWiring.php +++ b/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' ) ); }, diff --git a/includes/TrackingCategories.php b/includes/TrackingCategories.php index 690449b2179..f1f86904c60 100644 --- a/includes/TrackingCategories.php +++ b/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 */ - private $titleFactory; + /** @var TitleParser */ + 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; + } } diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index f9f2da64da0..311f1e2cf21 100644 --- a/includes/parser/Parser.php +++ b/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() + ); } /** diff --git a/includes/parser/ParserFactory.php b/includes/parser/ParserFactory.php index 2b3c3d86c04..e919be61a0d 100644 --- a/includes/parser/ParserFactory.php +++ b/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--; diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index 2d5c2739ae1..a20f584d067 100644 --- a/includes/parser/ParserOutput.php +++ b/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 ) { - wfDebug( __METHOD__ . ": Not adding tracking category $msg to special 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; - } + $trackingCategories = MediaWikiServices::getInstance()->getTrackingCategories(); + return $trackingCategories->addTrackingCategory( $this, $msg, $page ); } /** diff --git a/tests/phpunit/includes/TrackingCategoriesTest.php b/tests/phpunit/includes/TrackingCategoriesTest.php new file mode 100644 index 00000000000..2f03ca90015 --- /dev/null +++ b/tests/phpunit/includes/TrackingCategoriesTest.php @@ -0,0 +1,47 @@ +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() ); + } +} diff --git a/tests/phpunit/includes/parser/ParserTest.php b/tests/phpunit/includes/parser/ParserTest.php index eeecf9df5bb..55d591e44aa 100644 --- a/tests/phpunit/includes/parser/ParserTest.php +++ b/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 ) ]; } diff --git a/tests/phpunit/unit/includes/parser/ParserFactoryTest.php b/tests/phpunit/unit/includes/parser/ParserFactoryTest.php index 0089b6f271d..d3cae8300a5 100644 --- a/tests/phpunit/unit/includes/parser/ParserFactoryTest.php +++ b/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; }