From c6810d74d18437caff5a0af3efa4c42ec1811bb1 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 11 Jul 2018 12:13:18 -0400 Subject: [PATCH] Deprecate ContentHandler::makeParserOptions() Having a different ParserOptions for each content model isn't feasible in an MCR world. And the only thing using this was Wikibase, which has been fixed to do what it needs in a different way. Bug: T194263 Change-Id: I01373b29ee25fa9346c6b0317155be4ccdc8c515 --- RELEASE-NOTES-1.32 | 2 + includes/content/AbstractContent.php | 2 +- includes/content/ContentHandler.php | 19 +----- .../jobs/CategoryMembershipChangeJob.php | 16 ++--- includes/page/WikiPage.php | 4 +- includes/parser/ParserOptions.php | 47 +++++++++----- .../includes/content/WikitextContentTest.php | 4 +- .../includes/parser/ParserOptionsTest.php | 62 +++++++++++++++++++ 8 files changed, 113 insertions(+), 43 deletions(-) diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index 25ea4f6e3a..98bb03f406 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -248,6 +248,8 @@ because of Phabricator reports. OutputPage::showFileCopyError() and OutputPage::showUnexpectedValueError(). * The Replacer, DoubleReplacer, HashtableReplacer, and RegexlikeReplacer classes are now deprecated. Use a Closure instead. +* (T194263) ContentHandler::makeParserOptions() is deprecated. Use + WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead. === Other changes in 1.32 === * (T198811) The following tables have had their UNIQUE indexes turned into proper diff --git a/includes/content/AbstractContent.php b/includes/content/AbstractContent.php index 284cab2441..b6211b0604 100644 --- a/includes/content/AbstractContent.php +++ b/includes/content/AbstractContent.php @@ -501,7 +501,7 @@ abstract class AbstractContent implements Content { ParserOptions $options = null, $generateHtml = true ) { if ( $options === null ) { - $options = $this->getContentHandler()->makeParserOptions( 'canonical' ); + $options = ParserOptions::newCanonical( 'canonical' ); } $po = new ParserOutput(); diff --git a/includes/content/ContentHandler.php b/includes/content/ContentHandler.php index 3c5ee2663b..e628aef35a 100644 --- a/includes/content/ContentHandler.php +++ b/includes/content/ContentHandler.php @@ -1114,6 +1114,8 @@ abstract class ContentHandler { /** * Get parser options suitable for rendering and caching the article * + * @deprecated since 1.32, use WikiPage::makeParserOptions() or + * ParserOptions::newCanonical() instead. * @param IContextSource|User|string $context One of the following: * - IContextSource: Use the User and the Language of the provided * context @@ -1126,22 +1128,7 @@ abstract class ContentHandler { * @return ParserOptions */ public function makeParserOptions( $context ) { - global $wgContLang; - - if ( $context instanceof IContextSource ) { - $user = $context->getUser(); - $lang = $context->getLanguage(); - } elseif ( $context instanceof User ) { // settings per user (even anons) - $user = $context; - $lang = null; - } elseif ( $context === 'canonical' ) { // canonical settings - $user = new User; - $lang = $wgContLang; - } else { - throw new MWException( "Bad context for parser options: $context" ); - } - - return ParserOptions::newCanonical( $user, $lang ); + return ParserOptions::newCanonical( $context ); } /** diff --git a/includes/jobqueue/jobs/CategoryMembershipChangeJob.php b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php index 16640be42e..a0c70abe71 100644 --- a/includes/jobqueue/jobs/CategoryMembershipChangeJob.php +++ b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php @@ -173,7 +173,7 @@ class CategoryMembershipChangeJob extends Job { } // Parse the new revision and get the categories - $categoryChanges = $this->getExplicitCategoriesChanges( $title, $newRev, $oldRev ); + $categoryChanges = $this->getExplicitCategoriesChanges( $page, $newRev, $oldRev ); list( $categoryInserts, $categoryDeletes ) = $categoryChanges; if ( !$categoryInserts && !$categoryDeletes ) { return; // nothing to do @@ -203,7 +203,7 @@ class CategoryMembershipChangeJob extends Job { } private function getExplicitCategoriesChanges( - Title $title, Revision $newRev, Revision $oldRev = null + WikiPage $page, Revision $newRev, Revision $oldRev = null ) { // Inject the same timestamp for both revision parses to avoid seeing category changes // due to time-based parser functions. Inject the same page title for the parses too. @@ -213,10 +213,10 @@ class CategoryMembershipChangeJob extends Job { // assumes these updates are perfectly FIFO and that link tables are always // up to date, neither of which are true. $oldCategories = $oldRev - ? $this->getCategoriesAtRev( $title, $oldRev, $parseTimestamp ) + ? $this->getCategoriesAtRev( $page, $oldRev, $parseTimestamp ) : []; // Parse the new revision and get the categories - $newCategories = $this->getCategoriesAtRev( $title, $newRev, $parseTimestamp ); + $newCategories = $this->getCategoriesAtRev( $page, $newRev, $parseTimestamp ); $categoryInserts = array_values( array_diff( $newCategories, $oldCategories ) ); $categoryDeletes = array_values( array_diff( $oldCategories, $newCategories ) ); @@ -225,19 +225,19 @@ class CategoryMembershipChangeJob extends Job { } /** - * @param Title $title + * @param WikiPage $page * @param Revision $rev * @param string $parseTimestamp TS_MW * * @return string[] category names */ - private function getCategoriesAtRev( Title $title, Revision $rev, $parseTimestamp ) { + private function getCategoriesAtRev( WikiPage $page, Revision $rev, $parseTimestamp ) { $content = $rev->getContent(); - $options = $content->getContentHandler()->makeParserOptions( 'canonical' ); + $options = $page->makeParserOptions( 'canonical' ); $options->setTimestamp( $parseTimestamp ); // This could possibly use the parser cache if it checked the revision ID, // but that's more complicated than it's worth. - $output = $content->getParserOutput( $title, $rev->getId(), $options ); + $output = $content->getParserOutput( $page->getTitle(), $rev->getId(), $options ); // array keys will cast numeric category names to ints // so we need to cast them back to strings to avoid breaking things! diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index f34e894d72..7cc25bd3fc 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -1870,7 +1870,7 @@ class WikiPage implements Page, IDBAccessObject { /** * Get parser options suitable for rendering the primary article wikitext * - * @see ContentHandler::makeParserOptions + * @see ParserOptions::newCanonical * * @param IContextSource|User|string $context One of the following: * - IContextSource: Use the User and the Language of the provided @@ -1882,7 +1882,7 @@ class WikiPage implements Page, IDBAccessObject { * @return ParserOptions */ public function makeParserOptions( $context ) { - $options = $this->getContentHandler()->makeParserOptions( $context ); + $options = ParserOptions::newCanonical( $context ); if ( $this->getTitle()->isConversionTable() ) { // @todo ConversionTable should become a separate content model, so diff --git a/includes/parser/ParserOptions.php b/includes/parser/ParserOptions.php index 3a7a1d64bb..c8e68b2d52 100644 --- a/includes/parser/ParserOptions.php +++ b/includes/parser/ParserOptions.php @@ -41,6 +41,13 @@ use Wikimedia\ScopedCallback; */ class ParserOptions { + /** + * Flag indicating that newCanonical() accepts an IContextSource or the string 'canonical', for + * back-compat checks from extensions. + * @since 1.32 + */ + const HAS_NEWCANONICAL_FROM_CONTEXT = 1; + /** * Default values for all options that are relevant for caching. * @see self::getDefaults() @@ -930,8 +937,7 @@ class ParserOptions { /** * @warning For interaction with the parser cache, use - * WikiPage::makeParserOptions(), ContentHandler::makeParserOptions(), or - * ParserOptions::newCanonical() instead. + * WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead. * @param User|null $user * @param Language|null $lang */ @@ -957,8 +963,7 @@ class ParserOptions { /** * Get a ParserOptions object for an anonymous user * @warning For interaction with the parser cache, use - * WikiPage::makeParserOptions(), ContentHandler::makeParserOptions(), or - * ParserOptions::newCanonical() instead. + * WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead. * @since 1.27 * @return ParserOptions */ @@ -972,8 +977,7 @@ class ParserOptions { * Language will be taken from $wgLang. * * @warning For interaction with the parser cache, use - * WikiPage::makeParserOptions(), ContentHandler::makeParserOptions(), or - * ParserOptions::newCanonical() instead. + * WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead. * @param User $user * @return ParserOptions */ @@ -985,8 +989,7 @@ class ParserOptions { * Get a ParserOptions object from a given user and language * * @warning For interaction with the parser cache, use - * WikiPage::makeParserOptions(), ContentHandler::makeParserOptions(), or - * ParserOptions::newCanonical() instead. + * WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead. * @param User $user * @param Language $lang * @return ParserOptions @@ -999,8 +1002,7 @@ class ParserOptions { * Get a ParserOptions object from a IContextSource object * * @warning For interaction with the parser cache, use - * WikiPage::makeParserOptions(), ContentHandler::makeParserOptions(), or - * ParserOptions::newCanonical() instead. + * WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead. * @param IContextSource $context * @return ParserOptions */ @@ -1015,12 +1017,29 @@ class ParserOptions { * different from the canonical values used for caching. * * @since 1.30 - * @param User|null $user - * @param Language|StubObject|null $lang + * @since 1.32 Added string and IContextSource as options for the first parameter + * @param IContextSource|string|User|null $context + * - If an IContextSource, the options are initialized based on the source's User and Language. + * - If the string 'canonical', the options are initialized with an anonymous user and + * $wgContLang. + * - If a User or null, the options are initialized for that User (or $wgUser if null). + * 'userlang' is taken from the $userLang parameter, defaulting to $wgLang if that is null. + * @param Language|StubObject|null $userLang (see above) * @return ParserOptions */ - public static function newCanonical( User $user = null, $lang = null ) { - $ret = new ParserOptions( $user, $lang ); + public static function newCanonical( $context = null, $userLang = null ) { + if ( $context instanceof IContextSource ) { + $ret = self::newFromContext( $context ); + } elseif ( $context === 'canonical' ) { + $ret = self::newFromAnon(); + } elseif ( $context instanceof User || $context === null ) { + $ret = new self( $context, $userLang ); + } else { + throw new InvalidArgumentException( + '$context must be an IContextSource, the string "canonical", a User, or null' + ); + } + foreach ( self::getCanonicalOverrides() as $k => $v ) { $ret->setOption( $k, $v ); } diff --git a/tests/phpunit/includes/content/WikitextContentTest.php b/tests/phpunit/includes/content/WikitextContentTest.php index 1db6aab618..c78bc5bd9c 100644 --- a/tests/phpunit/includes/content/WikitextContentTest.php +++ b/tests/phpunit/includes/content/WikitextContentTest.php @@ -377,7 +377,7 @@ just a test" $wikitext = false; $redirectTarget = false; $content = $this->newContent( 'hello world.' ); - $options = $content->getContentHandler()->makeParserOptions( 'canonical' ); + $options = ParserOptions::newCanonical( 'canonical' ); $options->setRedirectTarget( $title ); $content->getParserOutput( $title, null, $options ); $this->assertEquals( 'hello world.', $wikitext, @@ -394,7 +394,7 @@ just a test" $content = $this->newContent( "#REDIRECT [[TestRedirectParserOption/redir]]\nhello redirect." ); - $options = $content->getContentHandler()->makeParserOptions( 'canonical' ); + $options = ParserOptions::newCanonical( 'canonical' ); $content->getParserOutput( $title, null, $options ); $this->assertEquals( 'hello redirect.', diff --git a/tests/phpunit/includes/parser/ParserOptionsTest.php b/tests/phpunit/includes/parser/ParserOptionsTest.php index 8c61b0323b..48205f4456 100644 --- a/tests/phpunit/includes/parser/ParserOptionsTest.php +++ b/tests/phpunit/includes/parser/ParserOptionsTest.php @@ -42,6 +42,68 @@ class ParserOptionsTest extends MediaWikiTestCase { parent::tearDown(); } + public function testNewCanonical() { + $wgUser = $this->getMutableTestUser()->getUser(); + $wgLang = Language::factory( 'fr' ); + $wgContLang = Language::factory( 'qqx' ); + + $this->setMwGlobals( [ + 'wgUser' => $wgUser, + 'wgLang' => $wgLang, + 'wgContLang' => $wgContLang, + ] ); + + $user = $this->getMutableTestUser()->getUser(); + $lang = Language::factory( 'de' ); + $lang2 = Language::factory( 'bug' ); + $context = new DerivativeContext( RequestContext::getMain() ); + $context->setUser( $user ); + $context->setLanguage( $lang ); + + // No parameters picks up $wgUser and $wgLang + $popt = ParserOptions::newCanonical(); + $this->assertSame( $wgUser, $popt->getUser() ); + $this->assertSame( $wgLang, $popt->getUserLangObj() ); + + // Just a user uses $wgLang + $popt = ParserOptions::newCanonical( $user ); + $this->assertSame( $user, $popt->getUser() ); + $this->assertSame( $wgLang, $popt->getUserLangObj() ); + + // Just a language uses $wgUser + $popt = ParserOptions::newCanonical( null, $lang ); + $this->assertSame( $wgUser, $popt->getUser() ); + $this->assertSame( $lang, $popt->getUserLangObj() ); + + // Passing both works + $popt = ParserOptions::newCanonical( $user, $lang ); + $this->assertSame( $user, $popt->getUser() ); + $this->assertSame( $lang, $popt->getUserLangObj() ); + + // Passing 'canonical' uses an anon and $wgContLang, and ignores + // any passed $userLang + $popt = ParserOptions::newCanonical( 'canonical' ); + $this->assertTrue( $popt->getUser()->isAnon() ); + $this->assertSame( $wgContLang, $popt->getUserLangObj() ); + $popt = ParserOptions::newCanonical( 'canonical', $lang2 ); + $this->assertSame( $wgContLang, $popt->getUserLangObj() ); + + // Passing an IContextSource uses the user and lang from it, and ignores + // any passed $userLang + $popt = ParserOptions::newCanonical( $context ); + $this->assertSame( $user, $popt->getUser() ); + $this->assertSame( $lang, $popt->getUserLangObj() ); + $popt = ParserOptions::newCanonical( $context, $lang2 ); + $this->assertSame( $lang, $popt->getUserLangObj() ); + + // Passing something else raises an exception + try { + $popt = ParserOptions::newCanonical( 'bogus' ); + $this->fail( 'Excpected exception not thrown' ); + } catch ( InvalidArgumentException $ex ) { + } + } + /** * @dataProvider provideIsSafeToCache * @param bool $expect Expected value -- 2.20.1