From: daniel Date: Thu, 15 Nov 2018 16:40:53 +0000 (+0100) Subject: Use stashed ParserOutput during saving. X-Git-Tag: 1.34.0-rc.0~3437^2 X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=8c5cb4b1a76cfef9cf4f62aac54ee537d59de560 Use stashed ParserOutput during saving. The code in DerivedPageDataUpdater that used the stashed ParserOutput from ApiStashEdit::checkCache was accidentally broken when RevisionRenderer was introduced in I871978bf79f67c9e7954fb3fc8528d6e365f2cc1. This is likely the cause for the degraded save timing noted in T205369. Bug: T205369 Change-Id: I6d8fdda73dccae08d18bfb528b948706f56ad2e0 --- diff --git a/includes/Revision/RenderedRevision.php b/includes/Revision/RenderedRevision.php index 6eee3c4cf6..c8f56e98de 100644 --- a/includes/Revision/RenderedRevision.php +++ b/includes/Revision/RenderedRevision.php @@ -159,6 +159,28 @@ class RenderedRevision implements SlotRenderingProvider { return $this->options; } + /** + * Sets a ParserOutput to be returned by getRevisionParserOutput(). + * + * @note For internal use by RevisionRenderer only! This method may be modified + * or removed without notice per the deprecation policy. + * + * @internal + * + * @param ParserOutput $output + */ + public function setRevisionParserOutput( ParserOutput $output ) { + $this->revisionOutput = $output; + + // If there is only one slot, we assume that the combined output is identical + // with the main slot's output. This is intended to prevent a redundant re-parse of + // the content in case getSlotParserOutput( SlotRecord::MAIN ) is called, for instance + // from ContentHandler::getSecondaryDataUpdates. + if ( $this->revision->getSlotRoles() === [ SlotRecord::MAIN ] ) { + $this->slotsOutput[ SlotRecord::MAIN ] = $output; + } + } + /** * @param array $hints Hints given as an associative array. Known keys: * - 'generate-html' => bool: Whether the caller is interested in output HTML (as opposed diff --git a/includes/Revision/RevisionRenderer.php b/includes/Revision/RevisionRenderer.php index e2e84b60ca..265ad133a4 100644 --- a/includes/Revision/RevisionRenderer.php +++ b/includes/Revision/RevisionRenderer.php @@ -82,6 +82,11 @@ class RevisionRenderer { * - 'audience' the audience to use for content access. Default is * RevisionRecord::FOR_PUBLIC if $forUser is not set, RevisionRecord::FOR_THIS_USER * if $forUser is set. Can be set to RevisionRecord::RAW to disable audience checks. + * - 'known-revision-output' a combined ParserOutput for the revision, perhaps from + * some cache. the caller is responsible for ensuring that the ParserOutput indeed + * matched the $rev and $options. This mechanism is intended as a temporary stop-gap, + * for the time until caches have been changed to store RenderedRevision states instead + * of ParserOutput objects. * * @return RenderedRevision|null The rendered revision, or null if the audience checks fails. */ @@ -133,6 +138,10 @@ class RevisionRenderer { $renderedRevision->setSaveParseLogger( $this->saveParseLogger ); + if ( isset( $hints['known-revision-output'] ) ) { + $renderedRevision->setRevisionParserOutput( $hints['known-revision-output'] ); + } + return $renderedRevision; } diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php index ad29f91e99..4556af0646 100644 --- a/includes/Storage/DerivedPageDataUpdater.php +++ b/includes/Storage/DerivedPageDataUpdater.php @@ -762,17 +762,6 @@ class DerivedPageDataUpdater implements IDBAccessObject { $stashedEdit = ApiStashEdit::checkCache( $title, $mainContent, $legacyUser ); } - if ( $stashedEdit ) { - /** @var ParserOutput $output */ - $output = $stashedEdit->output; - - // TODO: this should happen when stashing the ParserOutput, not now! - $output->setCacheTime( $stashedEdit->timestamp ); - - // TODO: MCR: allow output for all slots to be stashed. - $this->canonicalParserOutput = $output; - } - $userPopts = ParserOptions::newFromUserAndLang( $user, $this->contLang ); Hooks::run( 'ArticlePrepareTextForEdit', [ $wikiPage, $userPopts ] ); @@ -853,6 +842,27 @@ class DerivedPageDataUpdater implements IDBAccessObject { } else { $this->parentRevision = $parentRevision; } + + $renderHints = [ 'use-master' => $this->useMaster(), 'audience' => RevisionRecord::RAW ]; + + if ( $stashedEdit ) { + /** @var ParserOutput $output */ + $output = $stashedEdit->output; + + // TODO: this should happen when stashing the ParserOutput, not now! + $output->setCacheTime( $stashedEdit->timestamp ); + + $renderHints['known-revision-output'] = $output; + } + + // NOTE: we want a canonical rendering, so don't pass $this->user or ParserOptions + // NOTE: the revision is either new or current, so we can bypass audience checks. + $this->renderedRevision = $this->revisionRenderer->getRenderedRevision( + $this->revision, + null, + null, + $renderHints + ); } /** @@ -879,18 +889,7 @@ class DerivedPageDataUpdater implements IDBAccessObject { * @return RenderedRevision */ public function getRenderedRevision() { - if ( !$this->renderedRevision ) { - $this->assertPrepared( __METHOD__ ); - - // NOTE: we want a canonical rendering, so don't pass $this->user or ParserOptions - // NOTE: the revision is either new or current, so we can bypass audience checks. - $this->renderedRevision = $this->revisionRenderer->getRenderedRevision( - $this->revision, - null, - null, - [ 'use-master' => $this->useMaster(), 'audience' => RevisionRecord::RAW ] - ); - } + $this->assertPrepared( __METHOD__ ); return $this->renderedRevision; } @@ -1210,6 +1209,19 @@ class DerivedPageDataUpdater implements IDBAccessObject { // Prune any output that depends on the revision ID. if ( $this->renderedRevision ) { $this->renderedRevision->updateRevision( $revision ); + } else { + + // NOTE: we want a canonical rendering, so don't pass $this->user or ParserOptions + // NOTE: the revision is either new or current, so we can bypass audience checks. + $this->renderedRevision = $this->revisionRenderer->getRenderedRevision( + $this->revision, + null, + null, + [ 'use-master' => $this->useMaster(), 'audience' => RevisionRecord::RAW ] + ); + + // XXX: Since we presumably are dealing with the current revision, + // we could try to get the ParserOutput from the parser cache. } // TODO: optionally get ParserOutput from the ParserCache here. diff --git a/tests/phpunit/includes/Revision/RenderedRevisionTest.php b/tests/phpunit/includes/Revision/RenderedRevisionTest.php index 08a8fa6dd3..43fccee194 100644 --- a/tests/phpunit/includes/Revision/RenderedRevisionTest.php +++ b/tests/phpunit/includes/Revision/RenderedRevisionTest.php @@ -483,6 +483,23 @@ class RenderedRevisionTest extends MediaWikiTestCase { $this->assertContains( 'time:20180101000003!', $html ); } + public function testSetRevisionParserOutput() { + $title = $this->getMockTitle( 3, 21 ); + $rev = $this->getMockRevision( RevisionStoreRecord::class, $title ); + + $options = ParserOptions::newCanonical( 'canonical' ); + $rr = new RenderedRevision( $title, $rev, $options, $this->combinerCallback ); + + $output = new ParserOutput( 'Kittens' ); + $rr->setRevisionParserOutput( $output ); + + $this->assertSame( $output, $rr->getRevisionParserOutput() ); + $this->assertSame( 'Kittens', $rr->getRevisionParserOutput()->getText() ); + + $this->assertSame( $output, $rr->getSlotParserOutput( SlotRecord::MAIN ) ); + $this->assertSame( 'Kittens', $rr->getSlotParserOutput( SlotRecord::MAIN )->getText() ); + } + public function testNoHtml() { /** @var MockObject|Content $mockContent */ $mockContent = $this->getMockBuilder( WikitextContent::class ) diff --git a/tests/phpunit/includes/Revision/RevisionRendererTest.php b/tests/phpunit/includes/Revision/RevisionRendererTest.php index 469f2816dd..5c75edee9a 100644 --- a/tests/phpunit/includes/Revision/RevisionRendererTest.php +++ b/tests/phpunit/includes/Revision/RevisionRendererTest.php @@ -240,6 +240,34 @@ class RevisionRendererTest extends MediaWikiTestCase { $this->assertSame( $html, $rr->getSlotParserOutput( SlotRecord::MAIN )->getText() ); } + public function testGetRenderedRevision_known() { + $renderer = $this->newRevisionRenderer( 100, true ); // use master + $title = $this->getMockTitle( 7, 21 ); + + $rev = new MutableRevisionRecord( $title ); + $rev->setId( 21 ); // current! + $rev->setUser( new UserIdentityValue( 9, 'Frank', 0 ) ); + $rev->setTimestamp( '20180101000003' ); + $rev->setComment( CommentStoreComment::newUnsavedComment( '' ) ); + + $text = "uncached text"; + $rev->setContent( SlotRecord::MAIN, new WikitextContent( $text ) ); + + $output = new ParserOutput( 'cached text' ); + + $options = ParserOptions::newCanonical( 'canonical' ); + $rr = $renderer->getRenderedRevision( + $rev, + $options, + null, + [ 'known-revision-output' => $output ] + ); + + $this->assertSame( $output, $rr->getRevisionParserOutput() ); + $this->assertSame( 'cached text', $rr->getRevisionParserOutput()->getText() ); + $this->assertSame( 'cached text', $rr->getSlotParserOutput( SlotRecord::MAIN )->getText() ); + } + public function testGetRenderedRevision_old() { $renderer = $this->newRevisionRenderer( 100 ); $title = $this->getMockTitle( 7, 21 );