Anomie has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/360676 )
Change subject: Remove ParserOptions::legacyOptions() and cleanup related code ...................................................................... Remove ParserOptions::legacyOptions() and cleanup related code ParserOptions::legacyOptions() has been sitting around since 1.17. Originally it seems to have been intended as a way to avoid a mass cache invalidation (similar to optionsHashPre30() from I7fb9ffca9). That code was mostly removed in 1.23, but legacyOptions() was left behind because it was also being used in a few places as "all cache-varying options" (despite it not being documented for that purpose) where we'd rather have any key than no key at all. This patch creates an actual ParserOptions::allCacheVaryingOptions() method for those use cases and deprecates the long-obsolete legacyOptions(). It also makes more explicit the use of the "all cache-varying options" fallback in ParserCache::getKey(), and doesn't bother trying to use that fallback in ParserCache::get() where it no longer makes sense. Change-Id: Ife1e54744155136a570210c03fe907f18f8e8ece --- M includes/parser/ParserCache.php M includes/parser/ParserOptions.php M tests/phpunit/includes/parser/ParserOptionsTest.php 3 files changed, 120 insertions(+), 19 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/76/360676/1 diff --git a/includes/parser/ParserCache.php b/includes/parser/ParserCache.php index 76a7e1e..3323b3b 100644 --- a/includes/parser/ParserCache.php +++ b/includes/parser/ParserCache.php @@ -26,6 +26,26 @@ * @todo document */ class ParserCache { + /** + * Constants for self::getKey() + * @since 1.30 + */ + + /** Do not use expired or outdated data */ + const DO_NOT_USE = 0; + + /** Use expired data */ + const USE_EXPIRED = 1; + + /** Use expired data and data from different revisions */ + const USE_OUTDATED = 2; + + /** + * Use expired data and data from different revisions, and if all else + * fails vary on all variable options + */ + const USE_ANYTHING = 3; + /** @var BagOStuff */ private $mMemc; /** @@ -103,7 +123,7 @@ */ public function getETag( $article, $popts ) { return 'W/"' . $this->getParserOutputKey( $article, - $popts->optionsHash( ParserOptions::legacyOptions(), $article->getTitle() ) ) . + $popts->optionsHash( ParserOptions::allCacheVaryingOptions(), $article->getTitle() ) ) . "--" . $article->getTouched() . '"'; } @@ -130,15 +150,20 @@ * It would be preferable to have this code in get() * instead of having Article looking in our internals. * - * @todo Document parameter $useOutdated - * * @param WikiPage $article * @param ParserOptions $popts - * @param bool $useOutdated (default true) + * @param int|bool $useOutdated One of the USE constants. For backwards + * compatibility, boolean false is treated as DO_NOT_USE and + * boolean true is treated as USE_ANYTHING. * @return bool|mixed|string + * @since 1.30 Changed $useOutdated to an int and added the non-boolean values */ - public function getKey( $article, $popts, $useOutdated = true ) { + public function getKey( $article, $popts, $useOutdated = self::USE_ANYTHING ) { global $wgCacheEpoch; + + if ( is_bool( $useOutdated ) ) { + $useOutdated = $useOutdated ? self::USE_ANYTHING : self::DO_NOT_USE; + } if ( $popts instanceof User ) { wfWarn( "Use of outdated prototype ParserCache::getKey( &\$article, &\$user )\n" ); @@ -150,14 +175,16 @@ $optionsKey = $this->mMemc->get( $this->getOptionsKey( $article ), $casToken, BagOStuff::READ_VERIFIED ); if ( $optionsKey instanceof CacheTime ) { - if ( !$useOutdated && $optionsKey->expired( $article->getTouched() ) ) { + if ( $useOutdated < self::USE_EXPIRED && $optionsKey->expired( $article->getTouched() ) ) { wfIncrStats( "pcache.miss.expired" ); $cacheTime = $optionsKey->getCacheTime(); wfDebugLog( "ParserCache", "Parser options key expired, touched " . $article->getTouched() . ", epoch $wgCacheEpoch, cached $cacheTime\n" ); return false; - } elseif ( !$useOutdated && $optionsKey->isDifferentRevision( $article->getLatest() ) ) { + } elseif ( $useOutdated < self::USE_OUTDATED && + $optionsKey->isDifferentRevision( $article->getLatest() ) + ) { wfIncrStats( "pcache.miss.revid" ); $revId = $article->getLatest(); $cachedRevId = $optionsKey->getCacheRevisionId(); @@ -171,10 +198,10 @@ $usedOptions = $optionsKey->mUsedOptions; wfDebug( "Parser cache options found.\n" ); } else { - if ( !$useOutdated ) { + if ( $useOutdated < self::USE_ANYTHING ) { return false; } - $usedOptions = ParserOptions::legacyOptions(); + $usedOptions = ParserOptions::allCacheVaryingOptions(); } return $this->getParserOutputKey( @@ -204,7 +231,9 @@ $touched = $article->getTouched(); - $parserOutputKey = $this->getKey( $article, $popts, $useOutdated ); + $parserOutputKey = $this->getKey( $article, $popts, + $useOutdated ? self::USE_OUTDATED : self::DO_NOT_USE + ); if ( $parserOutputKey === false ) { wfIncrStats( 'pcache.miss.absent' ); return false; diff --git a/includes/parser/ParserOptions.php b/includes/parser/ParserOptions.php index 4809917..96a4368 100644 --- a/includes/parser/ParserOptions.php +++ b/includes/parser/ParserOptions.php @@ -1211,10 +1211,11 @@ * Returns the full array of options that would have been used by * in 1.16. * Used to get the old parser cache entries when available. - * @todo 1.16 was years ago, can we remove this? + * @deprecated since 1.30. You probably want self::allCacheVaryingOptions() instead. * @return array */ public static function legacyOptions() { + wfDeprecated( __METHOD__, '1.30' ); return [ 'stubthreshold', 'numberheadings', @@ -1226,6 +1227,20 @@ } /** + * Return all option keys that vary the options hash + * @since 1.30 + * @return string[] + */ + public static function allCacheVaryingOptions() { + // Trigger a call to the 'ParserOptionsRegister' hook if it hasn't + // already been called. + if ( self::$defaults === null ) { + self::getDefaults(); + } + return array_keys( array_filter( self::$inCacheKey ) ); + } + + /** * Convert an option to a string value * @param mixed $value * @return string diff --git a/tests/phpunit/includes/parser/ParserOptionsTest.php b/tests/phpunit/includes/parser/ParserOptionsTest.php index 264e35d..ad899bd 100644 --- a/tests/phpunit/includes/parser/ParserOptionsTest.php +++ b/tests/phpunit/includes/parser/ParserOptionsTest.php @@ -5,6 +5,42 @@ class ParserOptionsTest extends MediaWikiTestCase { + private static function clearCache() { + $wrap = TestingAccessWrapper::newFromClass( ParserOptions::class ); + $wrap->defaults = null; + $wrap->lazyOptions = [ + 'dateformat' => [ ParserOptions::class, 'initDateFormat' ], + ]; + $wrap->inCacheKey = [ + 'dateformat' => true, + 'numberheadings' => true, + 'thumbsize' => true, + 'stubthreshold' => true, + 'printable' => true, + 'userlang' => true, + 'wrapclass' => true, + ]; + } + + protected function setUp() { + global $wgHooks; + + parent::setUp(); + self::clearCache(); + + $this->setMwGlobals( [ + 'wgRenderHashAppend' => '', + 'wgHooks' => [ + 'PageRenderingHash' => [], + ] + $wgHooks, + ] ); + } + + protected function tearDown() { + self::clearCache(); + parent::tearDown(); + } + /** * @dataProvider provideIsSafeToCache * @param bool $expect Expected value @@ -48,7 +84,6 @@ global $wgHooks; $globals += [ - 'wgRenderHashAppend' => '', 'wgHooks' => [], ]; $globals['wgHooks'] += [ @@ -103,13 +138,6 @@ // Test weird historical behavior is still weird public function testOptionsHashEditSection() { - global $wgHooks; - - $this->setMwGlobals( [ - 'wgRenderHashAppend' => '', - 'wgHooks' => [ 'PageRenderingHash' => [] ] + $wgHooks, - ] ); - $popt = ParserOptions::newCanonical(); $popt->registerWatcher( function ( $name ) { $this->assertNotEquals( 'editsection', $name ); @@ -175,4 +203,33 @@ ScopedCallback::consume( $reset ); } + public function testAllCacheVaryingOptions() { + global $wgHooks; + + // $wgHooks is already saved in self::setUp(), so we can modify it freely here + $wgHooks['ParserOptionsRegister'] = []; + $this->assertSame( [ + 'dateformat', 'numberheadings', 'printable', 'stubthreshold', + 'thumbsize', 'userlang', 'wrapclass', + ], ParserOptions::allCacheVaryingOptions() ); + + self::clearCache(); + + $wgHooks['ParserOptionsRegister'][] = function ( &$defaults, &$inCacheKey ) { + $defaults += [ + 'foo' => 'foo', + 'bar' => 'bar', + 'baz' => 'baz', + ]; + $inCacheKey += [ + 'foo' => true, + 'bar' => false, + ]; + }; + $this->assertSame( [ + 'dateformat', 'foo', 'numberheadings', 'printable', 'stubthreshold', + 'thumbsize', 'userlang', 'wrapclass', + ], ParserOptions::allCacheVaryingOptions() ); + } + } -- To view, visit https://gerrit.wikimedia.org/r/360676 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ife1e54744155136a570210c03fe907f18f8e8ece Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Anomie <bjor...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits