From a227395e3ab67ee6fec64f904c4360fe8803b80b Mon Sep 17 00:00:00 2001 From: Adam Wight Date: Wed, 11 Dec 2019 11:01:05 +0100 Subject: [PATCH] Lazy instantiation of Cite Only create a Cite object if we need one. Never clearState, just destroy and recreate later. This makes it less likely that we leak state between parsers, and saves memory and processing on pages without references. It's also preparation to decouple Cite logic from state. Change-Id: I3db517591f4131c23151c76c223af7419cc00ae9 --- extension.json | 4 +-- src/Cite.php | 19 ------------- src/Hooks/CiteParserHooks.php | 31 +++++++--------------- src/Hooks/CiteParserTagHooks.php | 19 ++++++++++--- src/ReferenceStack.php | 10 ------- tests/phpunit/unit/CiteParserHooksTest.php | 29 ++++---------------- tests/phpunit/unit/ReferenceStackTest.php | 4 --- 7 files changed, 31 insertions(+), 85 deletions(-) diff --git a/extension.json b/extension.json index 08ebbb9a1..7b3ede6d5 100644 --- a/extension.json +++ b/extension.json @@ -28,8 +28,8 @@ "APIQuerySiteInfoGeneralInfo": "Cite\\Hooks\\CiteHooks::onAPIQuerySiteInfoGeneralInfo", "ContentHandlerDefaultModelFor": "Cite\\Hooks\\CiteHooks::onContentHandlerDefaultModelFor", "ParserAfterParse": "Cite\\Hooks\\CiteParserHooks::onParserAfterParse", - "ParserClearState": "Cite\\Hooks\\CiteParserHooks::onParserClearState", - "ParserCloned": "Cite\\Hooks\\CiteParserHooks::onParserCloned", + "ParserClearState": "Cite\\Hooks\\CiteParserHooks::onParserClearStateOrCloned", + "ParserCloned": "Cite\\Hooks\\CiteParserHooks::onParserClearStateOrCloned", "ParserFirstCallInit": "Cite\\Hooks\\CiteParserHooks::onParserFirstCallInit", "ResourceLoaderGetConfigVars": "Cite\\Hooks\\CiteHooks::onResourceLoaderGetConfigVars", "ResourceLoaderRegisterModules": "Cite\\Hooks\\CiteHooks::onResourceLoaderRegisterModules", diff --git a/src/Cite.php b/src/Cite.php index 6dbca07b4..d353cf7a0 100644 --- a/src/Cite.php +++ b/src/Cite.php @@ -508,25 +508,6 @@ class Cite { return $ret; } - /** - * Gets run when Parser::clearState() gets run, since we don't - * want the counts to transcend pages and other instances - * - * @param string $force Set to "force" to interrupt parsing - */ - public function clearState( string $force = '' ) { - if ( $force === 'force' ) { - $this->mInCite = false; - $this->inReferencesGroup = null; - } elseif ( $this->mInCite || $this->inReferencesGroup !== null ) { - // Don't clear when we're in the middle of parsing a or tag - return; - } - if ( $this->referenceStack ) { - $this->referenceStack->clear(); - } - } - /** * Called at the end of page processing to append a default references * section, if refs were used without a main references tag. If there are references diff --git a/src/Hooks/CiteParserHooks.php b/src/Hooks/CiteParserHooks.php index a554edadd..bc5798d22 100644 --- a/src/Hooks/CiteParserHooks.php +++ b/src/Hooks/CiteParserHooks.php @@ -17,34 +17,19 @@ class CiteParserHooks { * @param Parser $parser */ public static function onParserFirstCallInit( Parser $parser ) { - $parser->extCite = new Cite(); CiteParserTagHooks::register( $parser ); } /** * @see https://www.mediawiki.org/wiki/Manual:Hooks/ParserClearState - * - * @param Parser $parser - */ - public static function onParserClearState( Parser $parser ) { - /** @var Cite $cite */ - $cite = $parser->extCite; - $cite->clearState(); - } - - /** * @see https://www.mediawiki.org/wiki/Manual:Hooks/ParserCloned * * @param Parser $parser */ - public static function onParserCloned( Parser $parser ) { - $parser->extCite = clone $parser->extCite; - - /** @var Cite $cite */ - $cite = $parser->extCite; - $cite->clearState( 'force' ); - - CiteParserTagHooks::register( $parser ); + public static function onParserClearStateOrCloned( Parser $parser ) { + if ( isset( $parser->extCite ) ) { + unset( $parser->extCite ); + } } /** @@ -55,9 +40,11 @@ class CiteParserHooks { * @param StripState $stripState */ public static function onParserAfterParse( Parser $parser, &$text, $stripState ) { - /** @var Cite $cite */ - $cite = $parser->extCite; - $text .= $cite->checkRefsNoReferences( $parser->getOptions()->getIsSectionPreview() ); + if ( isset( $parser->extCite ) ) { + /** @var Cite $cite */ + $cite = $parser->extCite; + $text .= $cite->checkRefsNoReferences( $parser->getOptions()->getIsSectionPreview() ); + } } } diff --git a/src/Hooks/CiteParserTagHooks.php b/src/Hooks/CiteParserTagHooks.php index 4a65e2d3b..5563f8223 100644 --- a/src/Hooks/CiteParserTagHooks.php +++ b/src/Hooks/CiteParserTagHooks.php @@ -32,8 +32,7 @@ class CiteParserTagHooks { * @return string HTML */ public static function ref( $content, array $attributes, Parser $parser, PPFrame $frame ) { - /** @var Cite $cite */ - $cite = $parser->extCite; + $cite = self::citeForParser( $parser ); $result = $cite->ref( $content, $attributes, $parser ); if ( $result === false ) { @@ -60,8 +59,7 @@ class CiteParserTagHooks { * @return string HTML */ public static function references( $content, array $attributes, Parser $parser, PPFrame $frame ) { - /** @var Cite $cite */ - $cite = $parser->extCite; + $cite = self::citeForParser( $parser ); $result = $cite->references( $content, $attributes, $parser ); if ( $result === false ) { @@ -76,4 +74,17 @@ class CiteParserTagHooks { return $result; } + /** + * Get or create Cite state for this parser. + * + * @param Parser $parser + * @return Cite + */ + private static function citeForParser( Parser $parser ): Cite { + if ( !isset( $parser->extCite ) ) { + $parser->extCite = new Cite(); + } + return $parser->extCite; + } + } diff --git a/src/ReferenceStack.php b/src/ReferenceStack.php index ef77d0727..6499c8dd2 100644 --- a/src/ReferenceStack.php +++ b/src/ReferenceStack.php @@ -347,16 +347,6 @@ class ReferenceStack { } } - /** - * Reset all state. - */ - public function clear() { - $this->groupRefSequence = []; - $this->refSequence = 0; - $this->refs = []; - $this->refCallStack = []; - } - /** * Clear state for a single group. * diff --git a/tests/phpunit/unit/CiteParserHooksTest.php b/tests/phpunit/unit/CiteParserHooksTest.php index 238263e91..2054fcbc5 100644 --- a/tests/phpunit/unit/CiteParserHooksTest.php +++ b/tests/phpunit/unit/CiteParserHooksTest.php @@ -29,38 +29,19 @@ class CiteParserHooksTest extends \MediaWikiUnitTestCase { ); CiteParserHooks::onParserFirstCallInit( $parser ); - - $this->assertInstanceOf( Cite::class, $parser->extCite ); } /** - * @covers ::onParserClearState - */ - public function testOnParserClearState() { - $cite = $this->createMock( Cite::class ); - $cite->expects( $this->once() ) - ->method( 'clearState' ); - - $parser = $this->createMock( Parser::class ); - $parser->extCite = $cite; - - CiteParserHooks::onParserClearState( $parser ); - } - - /** - * @covers ::onParserCloned + * @covers ::onParserClearStateOrCloned */ public function testOnParserCloned() { - $cite = $this->createMock( Cite::class ); - $cite->expects( $this->once() ) - ->method( 'clearState' ); - $parser = $this->createMock( Parser::class ); - $parser->extCite = $cite; + $parser->extCite = $this->createMock( Cite::class ); - CiteParserHooks::onParserCloned( $parser ); + /** @var Parser $parser */ + CiteParserHooks::onParserClearStateOrCloned( $parser ); - $this->assertNotSame( $cite, $parser->extCite ); + $this->assertFalse( isset( $parser->extCite ) ); } /** diff --git a/tests/phpunit/unit/ReferenceStackTest.php b/tests/phpunit/unit/ReferenceStackTest.php index aef5a83e4..0ae10b6d0 100644 --- a/tests/phpunit/unit/ReferenceStackTest.php +++ b/tests/phpunit/unit/ReferenceStackTest.php @@ -1009,7 +1009,6 @@ class ReferenceStackTest extends MediaWikiUnitTestCase { } /** - * @covers ::clear * @covers ::deleteGroup */ public function testRemovals() { @@ -1018,9 +1017,6 @@ class ReferenceStackTest extends MediaWikiUnitTestCase { $stack->deleteGroup( 'group1' ); $this->assertSame( [ 'group2' => [] ], $stack->refs ); - - $stack->clear(); - $this->assertSame( [], $stack->refs ); } /**