From 84694a9d592ee5f93e41a5e7dc06eaa40c669c0c Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 21 Jun 2017 12:21:45 -0400 Subject: [PATCH] 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 --- includes/parser/ParserCache.php | 49 ++++++++++--- includes/parser/ParserOptions.php | 17 ++++- .../includes/parser/ParserOptionsTest.php | 73 +++++++++++++++++-- 3 files changed, 120 insertions(+), 19 deletions(-) diff --git a/includes/parser/ParserCache.php b/includes/parser/ParserCache.php index 76a7e1ed7a..3b84c4b077 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 + */ + + /** Use only current data */ + const USE_CURRENT_ONLY = 0; + + /** Use expired data if current data is unavailable */ + const USE_EXPIRED = 1; + + /** Use expired data or data from different revisions if current data is unavailable */ + 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 @@ class ParserCache { */ 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,16 +150,21 @@ class ParserCache { * 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 USE_CURRENT_ONLY 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::USE_CURRENT_ONLY; + } + if ( $popts instanceof User ) { wfWarn( "Use of outdated prototype ParserCache::getKey( &\$article, &\$user )\n" ); $popts = ParserOptions::newFromUser( $popts ); @@ -150,14 +175,16 @@ class ParserCache { $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 @@ class ParserCache { $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 @@ class ParserCache { $touched = $article->getTouched(); - $parserOutputKey = $this->getKey( $article, $popts, $useOutdated ); + $parserOutputKey = $this->getKey( $article, $popts, + $useOutdated ? self::USE_OUTDATED : self::USE_CURRENT_ONLY + ); if ( $parserOutputKey === false ) { wfIncrStats( 'pcache.miss.absent' ); return false; diff --git a/includes/parser/ParserOptions.php b/includes/parser/ParserOptions.php index 4809917b63..96a4368258 100644 --- a/includes/parser/ParserOptions.php +++ b/includes/parser/ParserOptions.php @@ -1211,10 +1211,11 @@ class ParserOptions { * 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', @@ -1225,6 +1226,20 @@ class ParserOptions { ]; } + /** + * 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 diff --git a/tests/phpunit/includes/parser/ParserOptionsTest.php b/tests/phpunit/includes/parser/ParserOptionsTest.php index 264e35d5a2..ad899bd7a9 100644 --- a/tests/phpunit/includes/parser/ParserOptionsTest.php +++ b/tests/phpunit/includes/parser/ParserOptionsTest.php @@ -5,6 +5,42 @@ use Wikimedia\ScopedCallback; 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 @@ class ParserOptionsTest extends MediaWikiTestCase { global $wgHooks; $globals += [ - 'wgRenderHashAppend' => '', 'wgHooks' => [], ]; $globals['wgHooks'] += [ @@ -103,13 +138,6 @@ class ParserOptionsTest extends MediaWikiTestCase { // 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 @@ class ParserOptionsTest extends MediaWikiTestCase { 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() ); + } + } -- 2.20.1