From 46c0c39514298d2ed119e6665a180d023eed4207 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Sun, 11 Jun 2017 10:49:32 -0400 Subject: [PATCH] ParserOptions: Fix handling of 'editsection' The handling of the 'editsection' option prior to I7fb9ffca9 was unusual: it was included in the cache key, but the getter didn't ever flag it as "used". This was overlooked in I7fb9ffca9. This fixes the handling to restore that behavior. It's no longer considered to be a real parser option, so changing it won't make isSafeToCache() fail while reading it won't flag it as 'used'. But to keep Wikibase working (see T85252), if 'editsection' is supplied in $forOptions optionsHash() will still include it in the hash so whatever Wikibase is doing by forcing that doesn't break. The hash when it is included is the same as was used in I7fb9ffca9 to reuse keys. Once optionsHashPre30() is removed, Wikibase should be changed to use some other method to fix T85252 so we can remove that hack from optionsHash(). Change-Id: I77b5519c5a1122a1fafbfc523b77b2268c0efeb1 --- includes/parser/ParserOptions.php | 67 ++++++++++++------- .../includes/parser/ParserOptionsTest.php | 41 ++++++++++-- 2 files changed, 80 insertions(+), 28 deletions(-) diff --git a/includes/parser/ParserOptions.php b/includes/parser/ParserOptions.php index f8ed63fc84..5be0321bbe 100644 --- a/includes/parser/ParserOptions.php +++ b/includes/parser/ParserOptions.php @@ -60,7 +60,6 @@ class ParserOptions { */ private static $inCacheKey = [ 'dateformat' => true, - 'editsection' => true, 'numberheadings' => true, 'thumbsize' => true, 'stubthreshold' => true, @@ -82,6 +81,13 @@ class ParserOptions { */ private $mTimestamp; + /** + * The edit section flag is in ParserOptions for historical reasons, but + * doesn't actually affect the parser output since Feb 2015. + * @var bool + */ + private $mEditSection = true; + /** * Stored user object * @var User @@ -244,23 +250,6 @@ class ParserOptions { return $this->setOptionLegacy( 'enableImageWhitelist', $x ); } - /** - * Create "edit section" links? - * @return bool - */ - public function getEditSection() { - return $this->getOption( 'editsection' ); - } - - /** - * Create "edit section" links? - * @param bool|null $x New value (null is no change) - * @return bool Old value - */ - public function setEditSection( $x ) { - return $this->setOptionLegacy( 'editsection', $x ); - } - /** * Automatically number headings? * @return bool @@ -878,6 +867,23 @@ class ParserOptions { return wfSetVar( $this->mTimestamp, $x ); } + /** + * Create "edit section" links? + * @return bool + */ + public function getEditSection() { + return $this->mEditSection; + } + + /** + * Create "edit section" links? + * @param bool|null $x New value (null is no change) + * @return bool Old value + */ + public function setEditSection( $x ) { + return wfSetVar( $this->mEditSection, $x ); + } + /** * Set the redirect target. * @@ -1041,7 +1047,6 @@ class ParserOptions { // *UPDATE* ParserOptions::matches() if any of this changes as needed self::$defaults = [ 'dateformat' => null, - 'editsection' => true, 'tidy' => false, 'interfaceMessage' => false, 'targetLanguage' => null, @@ -1256,16 +1261,32 @@ class ParserOptions { public function optionsHash( $forOptions, $title = null ) { global $wgRenderHashAppend; + $options = $this->options; + $defaults = self::getCanonicalOverrides() + self::getDefaults(); + $inCacheKey = self::$inCacheKey; + + // Historical hack: 'editsection' hasn't been a true parser option since + // Feb 2015 (instead the parser outputs a constant placeholder and post-parse + // processing handles the option). But Wikibase forces it in $forOptions + // and expects the cache key to still vary on it for T85252. + // @todo Deprecate and remove this behavior after optionsHashPre30() is + // removed (Wikibase can use addExtraKey() or something instead). + if ( in_array( 'editsection', $forOptions, true ) ) { + $options['editsection'] = $this->mEditSection; + $defaults['editsection'] = true; + $inCacheKey['editsection'] = true; + ksort( $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. // The drawback to this is that changing the default value of an option // requires manual invalidation of existing cache entries, as mentioned // in the docs on the relevant methods and hooks. - $defaults = self::getCanonicalOverrides() + self::getDefaults(); $values = []; - foreach ( self::$inCacheKey as $option => $include ) { + foreach ( $inCacheKey as $option => $include ) { if ( $include && in_array( $option, $forOptions, true ) ) { - $v = $this->optionToString( $this->options[$option] ); + $v = $this->optionToString( $options[$option] ); $d = $this->optionToString( $defaults[$option] ); if ( $v !== $d ) { $values[] = "$option=$v"; @@ -1364,7 +1385,7 @@ class ParserOptions { // directly. At least Wikibase does at this point in time. if ( !in_array( 'editsection', $forOptions ) ) { $confstr .= '!*'; - } elseif ( !$this->options['editsection'] ) { + } elseif ( !$this->mEditSection ) { $confstr .= '!edit=0'; } diff --git a/tests/phpunit/includes/parser/ParserOptionsTest.php b/tests/phpunit/includes/parser/ParserOptionsTest.php index 81f0564024..d6061420e7 100644 --- a/tests/phpunit/includes/parser/ParserOptionsTest.php +++ b/tests/phpunit/includes/parser/ParserOptionsTest.php @@ -22,7 +22,6 @@ class ParserOptionsTest extends MediaWikiTestCase { return [ 'No overrides' => [ true, [] ], 'In-key options are ok' => [ true, [ - 'editsection' => false, 'thumbsize' => 1e100, 'wrapclass' => false, ] ], @@ -65,14 +64,14 @@ class ParserOptionsTest extends MediaWikiTestCase { } public static function provideOptionsHashPre30() { - $used = [ 'wrapclass', 'editsection', 'printable' ]; + $used = [ 'wrapclass', 'printable' ]; return [ 'Canonical options, nothing used' => [ [], '*!*!*!*!*!*', [] ], - 'Canonical options, used some options' => [ $used, '*!*!*!*!*', [] ], + 'Canonical options, used some options' => [ $used, '*!*!*!*!*!*', [] ], 'Used some options, non-default values' => [ $used, - '*!*!*!*!*!printable=1!wrapclass=foobar', + '*!*!*!*!*!*!printable=1!wrapclass=foobar', [ 'setWrapOutputClass' => 'foobar', 'setIsPrintable' => true, @@ -87,6 +86,14 @@ class ParserOptionsTest extends MediaWikiTestCase { 'wgHooks' => [ 'PageRenderingHash' => [ [ __CLASS__ . '::onPageRenderingHash' ] ] ], ] ], + + // Test weird historical behavior is still weird + 'Canonical options, editsection=true used' => [ [ 'editsection' ], '*!*!*!*!*', [ + 'setEditSection' => true, + ] ], + 'Canonical options, editsection=false used' => [ [ 'editsection' ], '*!*!*!*!*!edit=0', [ + 'setEditSection' => false, + ] ], ]; } @@ -117,7 +124,7 @@ class ParserOptionsTest extends MediaWikiTestCase { } public static function provideOptionsHash() { - $used = [ 'wrapclass', 'editsection', 'printable' ]; + $used = [ 'wrapclass', 'printable' ]; $classWrapper = TestingAccessWrapper::newFromClass( ParserOptions::class ); $classWrapper->getDefaults(); @@ -154,6 +161,30 @@ class ParserOptionsTest extends MediaWikiTestCase { $confstr .= '!onPageRenderingHash'; } + // 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 ); + } ); + + $this->assertTrue( $popt->getEditSection() ); + $this->assertSame( 'canonical', $popt->optionsHash( [] ) ); + $this->assertSame( 'canonical', $popt->optionsHash( [ 'editsection' ] ) ); + + $popt->setEditSection( false ); + $this->assertFalse( $popt->getEditSection() ); + $this->assertSame( 'canonical', $popt->optionsHash( [] ) ); + $this->assertSame( 'editsection=0', $popt->optionsHash( [ 'editsection' ] ) ); + } + /** * @expectedException InvalidArgumentException * @expectedExceptionMessage Unknown parser option bogus -- 2.20.1