From c3900e06becc6b829726cfe133bb6c680deb993d Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Mon, 26 Mar 2018 13:59:24 -0400 Subject: [PATCH] Resolve used lazy options in ParserOptions::optionsHash() If a lazy option is passed to ParserOptions::optionsHash(), we should resolve the option so the hash can incorporate the proper value instead of omitting it. Also, completely unrelatedly, refactor the hook overriding in the unit test because people won't stop whining about it in code review. Change-Id: I2df78ed90875c229090b503b65f20fbbbba7f237 --- includes/parser/ParserOptions.php | 22 ++++-- .../includes/parser/ParserOptionsTest.php | 76 +++++++++++++------ 2 files changed, 66 insertions(+), 32 deletions(-) diff --git a/includes/parser/ParserOptions.php b/includes/parser/ParserOptions.php index 8fb98579f2..20bd599925 100644 --- a/includes/parser/ParserOptions.php +++ b/includes/parser/ParserOptions.php @@ -1275,9 +1275,17 @@ class ParserOptions { public function optionsHash( $forOptions, $title = null ) { global $wgRenderHashAppend; + $inCacheKey = self::allCacheVaryingOptions(); + + // Resolve any lazy options + foreach ( array_intersect( $forOptions, $inCacheKey, array_keys( self::$lazyOptions ) ) as $k ) { + if ( $this->options[$k] === null ) { + $this->options[$k] = call_user_func( self::$lazyOptions[$k], $this, $k ); + } + } + $options = $this->options; $defaults = self::getCanonicalOverrides() + self::getDefaults(); - $inCacheKey = self::$inCacheKey; // We only include used options with non-canonical values in the key // so adding a new option doesn't invalidate the entire parser cache. @@ -1285,13 +1293,11 @@ class ParserOptions { // requires manual invalidation of existing cache entries, as mentioned // in the docs on the relevant methods and hooks. $values = []; - foreach ( $inCacheKey as $option => $include ) { - if ( $include && in_array( $option, $forOptions, true ) ) { - $v = $this->optionToString( $options[$option] ); - $d = $this->optionToString( $defaults[$option] ); - if ( $v !== $d ) { - $values[] = "$option=$v"; - } + foreach ( array_intersect( $inCacheKey, $forOptions ) as $option ) { + $v = $this->optionToString( $options[$option] ); + $d = $this->optionToString( $defaults[$option] ); + if ( $v !== $d ) { + $values[] = "$option=$v"; } } diff --git a/tests/phpunit/includes/parser/ParserOptionsTest.php b/tests/phpunit/includes/parser/ParserOptionsTest.php index e2ed1d57e0..8c61b0323b 100644 --- a/tests/phpunit/includes/parser/ParserOptionsTest.php +++ b/tests/phpunit/includes/parser/ParserOptionsTest.php @@ -25,17 +25,16 @@ class ParserOptionsTest extends MediaWikiTestCase { } protected function setUp() { - global $wgHooks; - parent::setUp(); self::clearCache(); $this->setMwGlobals( [ 'wgRenderHashAppend' => '', - 'wgHooks' => [ - 'PageRenderingHash' => [], - ] + $wgHooks, ] ); + + // This is crazy, but registering false, null, or other falsey values + // as a hook callback "works". + $this->setTemporaryHook( 'PageRenderingHash', null ); } protected function tearDown() { @@ -84,17 +83,13 @@ class ParserOptionsTest extends MediaWikiTestCase { * @param string $expect Expected value * @param array $options Options to set * @param array $globals Globals to set + * @param callable|null $hookFunc PageRenderingHash hook function */ - public function testOptionsHash( $usedOptions, $expect, $options, $globals = [] ) { - global $wgHooks; - - $globals += [ - 'wgHooks' => [], - ]; - $globals['wgHooks'] += [ - 'PageRenderingHash' => [], - ] + $wgHooks; + public function testOptionsHash( + $usedOptions, $expect, $options, $globals = [], $hookFunc = null + ) { $this->setMwGlobals( $globals ); + $this->setTemporaryHook( 'PageRenderingHash', $hookFunc ); $popt = ParserOptions::newCanonical(); foreach ( $options as $name => $value ) { @@ -129,14 +124,50 @@ class ParserOptionsTest extends MediaWikiTestCase { [], 'canonical!wgRenderHashAppend!onPageRenderingHash', [], - [ - 'wgRenderHashAppend' => '!wgRenderHashAppend', - 'wgHooks' => [ 'PageRenderingHash' => [ [ __CLASS__ . '::onPageRenderingHash' ] ] ], - ] + [ 'wgRenderHashAppend' => '!wgRenderHashAppend' ], + [ __CLASS__ . '::onPageRenderingHash' ], ], ]; } + public function testUsedLazyOptionsInHash() { + $this->setTemporaryHook( 'ParserOptionsRegister', + function ( &$defaults, &$inCacheKey, &$lazyOptions ) { + $lazyFuncs = $this->getMockBuilder( stdClass::class ) + ->setMethods( [ 'neverCalled', 'calledOnce' ] ) + ->getMock(); + $lazyFuncs->expects( $this->never() )->method( 'neverCalled' ); + $lazyFuncs->expects( $this->once() )->method( 'calledOnce' )->willReturn( 'value' ); + + $defaults += [ + 'opt1' => null, + 'opt2' => null, + 'opt3' => null, + ]; + $inCacheKey += [ + 'opt1' => true, + 'opt2' => true, + ]; + $lazyOptions += [ + 'opt1' => [ $lazyFuncs, 'calledOnce' ], + 'opt2' => [ $lazyFuncs, 'neverCalled' ], + 'opt3' => [ $lazyFuncs, 'neverCalled' ], + ]; + } + ); + + self::clearCache(); + + $popt = ParserOptions::newCanonical(); + $popt->registerWatcher( function () { + $this->fail( 'Watcher should not have been called' ); + } ); + $this->assertSame( 'opt1=value', $popt->optionsHash( [ 'opt1', 'opt3' ] ) ); + + // Second call to see that opt1 isn't resolved a second time + $this->assertSame( 'opt1=value', $popt->optionsHash( [ 'opt1', 'opt3' ] ) ); + } + public static function onPageRenderingHash( &$confstr ) { $confstr .= '!onPageRenderingHash'; } @@ -192,10 +223,7 @@ class ParserOptionsTest extends MediaWikiTestCase { } public function testAllCacheVaryingOptions() { - global $wgHooks; - - // $wgHooks is already saved in self::setUp(), so we can modify it freely here - $wgHooks['ParserOptionsRegister'] = []; + $this->setTemporaryHook( 'ParserOptionsRegister', null ); $this->assertSame( [ 'dateformat', 'numberheadings', 'printable', 'stubthreshold', 'thumbsize', 'userlang' @@ -203,7 +231,7 @@ class ParserOptionsTest extends MediaWikiTestCase { self::clearCache(); - $wgHooks['ParserOptionsRegister'][] = function ( &$defaults, &$inCacheKey ) { + $this->setTemporaryHook( 'ParserOptionsRegister', function ( &$defaults, &$inCacheKey ) { $defaults += [ 'foo' => 'foo', 'bar' => 'bar', @@ -213,7 +241,7 @@ class ParserOptionsTest extends MediaWikiTestCase { 'foo' => true, 'bar' => false, ]; - }; + } ); $this->assertSame( [ 'dateformat', 'foo', 'numberheadings', 'printable', 'stubthreshold', 'thumbsize', 'userlang' -- 2.20.1