From d8c409dd161e248b8ac5948528c02ab6c877c706 Mon Sep 17 00:00:00 2001 From: daniel Date: Mon, 13 Aug 2018 22:33:31 +0200 Subject: [PATCH] Make HTML generation in RenderedRevision optional This allows optimization for situations in which a caller needs the meta-data of a ParserOutput, and the respective ContentHandler can provide that meta-data without generating HTML output. Bug: T194048 Change-Id: I786d294d18a6a2e3cea61577313e21b578c44f1e --- includes/Revision/RenderedRevision.php | 40 +++++++-- includes/Revision/RevisionRenderer.php | 59 +++++++------ includes/Storage/DerivedPageDataUpdater.php | 6 +- includes/parser/ParserCache.php | 4 + includes/parser/ParserOutput.php | 33 +++++++- .../Revision/RenderedRevisionTest.php | 82 +++++++++++++++---- .../Revision/RevisionRendererTest.php | 37 +++++++++ .../includes/parser/ParserOutputTest.php | 38 +++++++++ 8 files changed, 245 insertions(+), 54 deletions(-) diff --git a/includes/Revision/RenderedRevision.php b/includes/Revision/RenderedRevision.php index 380cfadc0c..0c052d1b46 100644 --- a/includes/Revision/RenderedRevision.php +++ b/includes/Revision/RenderedRevision.php @@ -157,11 +157,19 @@ class RenderedRevision { } /** + * @param array $hints Hints given as an associative array. Known keys: + * - 'generate-html' => bool: Whether the caller is interested in output HTML (as opposed + * to just meta-data). Default is to generate HTML. + * * @return ParserOutput */ - public function getRevisionParserOutput() { - if ( !$this->revisionOutput ) { - $output = call_user_func( $this->combineOutput, $this ); + public function getRevisionParserOutput( array $hints = [] ) { + $withHtml = $hints['generate-html'] ?? true; + + if ( !$this->revisionOutput + || ( $withHtml && !$this->revisionOutput->hasText() ) + ) { + $output = call_user_func( $this->combineOutput, $this, $hints ); Assert::postcondition( $output instanceof ParserOutput, @@ -176,15 +184,20 @@ class RenderedRevision { /** * @param string $role + * @param array $hints Hints given as an associative array. Known keys: + * - 'generate-html' => bool: Whether the caller is interested in output HTML (as opposed + * to just meta-data). Default is to generate HTML. * * @throws SuppressedDataException if the content is not accessible for the audience * specified in the constructor. * @return ParserOutput */ - public function getSlotParserOutput( $role ) { - // XXX: make html generation optional? + public function getSlotParserOutput( $role, array $hints = [] ) { + $withHtml = $hints['generate-html'] ?? true; - if ( !isset( $this->slotsOutput[$role] ) ) { + if ( !isset( $this->slotsOutput[ $role ] ) + || ( $withHtml && !$this->slotsOutput[ $role ]->hasText() ) + ) { $content = $this->revision->getContent( $role, $this->audience, $this->forUser ); if ( !$content ) { @@ -192,15 +205,26 @@ class RenderedRevision { 'Access to the content has been suppressed for this audience' ); } else { - $this->slotsOutput[ $role ] = $content->getParserOutput( + $output = $content->getParserOutput( $this->title, $this->revision->getId(), - $this->options + $this->options, + $withHtml ); + if ( $withHtml && !$output->hasText() ) { + throw new LogicException( + 'HTML generation was requested, but ' + . get_class( $content ) + . '::getParserOutput() returns a ParserOutput with no text set.' + ); + } + // Detach watcher, to ensure option use is not recorded in the wrong ParserOutput. $this->options->registerWatcher( null ); } + + $this->slotsOutput[ $role ] = $output; } return $this->slotsOutput[$role]; diff --git a/includes/Revision/RevisionRenderer.php b/includes/Revision/RevisionRenderer.php index 8f44a19f40..f71f9e71d7 100644 --- a/includes/Revision/RevisionRenderer.php +++ b/includes/Revision/RevisionRenderer.php @@ -118,8 +118,8 @@ class RevisionRenderer { $title, $rev, $options, - function ( RenderedRevision $rrev ) { - return $this->combineSlotOutput( $rrev ); + function ( RenderedRevision $rrev, array $hints ) { + return $this->combineSlotOutput( $rrev, $hints ); }, $audience, $forUser @@ -154,12 +154,16 @@ class RevisionRenderer { * @todo Use placement hints from SlotRoleHandlers instead of hard-coding the layout. * * @param RenderedRevision $rrev + * @param array $hints see RenderedRevision::getRevisionParserOutput() + * * @return ParserOutput */ - private function combineSlotOutput( RenderedRevision $rrev ) { + private function combineSlotOutput( RenderedRevision $rrev, array $hints = [] ) { $revision = $rrev->getRevision(); $slots = $revision->getSlots()->getSlots(); + $withHtml = $hints['generate-html'] ?? true; + // short circuit if there is only the main slot if ( array_keys( $slots ) === [ 'main' ] ) { return $rrev->getSlotParserOutput( 'main' ); @@ -172,36 +176,43 @@ class RevisionRenderer { $slots = [ 'main' => $slots['main'] ] + $slots; } - $output = new ParserOutput(); + $combinedOutput = new ParserOutput( null ); + $slotOutput = []; + $options = $rrev->getOptions(); - $options->registerWatcher( [ $output, 'recordOption' ] ); + $options->registerWatcher( [ $combinedOutput, 'recordOption' ] ); - $html = ''; - $first = true; foreach ( $slots as $role => $slot ) { + $out = $rrev->getSlotParserOutput( $role, $hints ); + $slotOutput[$role] = $out; - if ( $first ) { - // skip header for the first slot - $first = false; - } else { - // NOTE: this placeholder is hydrated by ParserOutput::getText(). - $headText = Html::element( 'mw:slotheader', [], $role ); - $html .= Html::rawElement( 'h1', [ 'class' => 'mw-slot-header' ], $headText ); - } - - $slotOutput = $rrev->getSlotParserOutput( $role ); + $combinedOutput->mergeInternalMetaDataFrom( $out, $role ); + $combinedOutput->mergeTrackingMetaDataFrom( $out ); + } - $html .= $slotOutput->getRawText(); + if ( $withHtml ) { + $html = ''; + $first = true; + /** @var ParserOutput $out */ + foreach ( $slotOutput as $role => $out ) { + if ( $first ) { + // skip header for the first slot + $first = false; + } else { + // NOTE: this placeholder is hydrated by ParserOutput::getText(). + $headText = Html::element( 'mw:slotheader', [], $role ); + $html .= Html::rawElement( 'h1', [ 'class' => 'mw-slot-header' ], $headText ); + } + + $html .= $out->getRawText(); + $combinedOutput->mergeHtmlMetaDataFrom( $out ); + } - $output->mergeInternalMetaDataFrom( $slotOutput ); - $output->mergeHtmlMetaDataFrom( $slotOutput ); - $output->mergeTrackingMetaDataFrom( $slotOutput ); + $combinedOutput->setText( $html ); } - $output->setText( $html ); - $options->registerWatcher( null ); - return $output; + return $combinedOutput; } } diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php index 736b0ca0e2..2df1670c88 100644 --- a/includes/Storage/DerivedPageDataUpdater.php +++ b/includes/Storage/DerivedPageDataUpdater.php @@ -1190,8 +1190,10 @@ class DerivedPageDataUpdater implements IDBAccessObject { * @return ParserOutput */ public function getSlotParserOutput( $role, $generateHtml = true ) { - // XXX: $generateHtml is currently ignored. RenderedRevision could be made to use it. - return $this->getRenderedRevision()->getSlotParserOutput( $role ); + return $this->getRenderedRevision()->getSlotParserOutput( + $role, + [ 'generate-html' => $generateHtml ] + ); } /** diff --git a/includes/parser/ParserCache.php b/includes/parser/ParserCache.php index 5e6081d33d..43c72b18f8 100644 --- a/includes/parser/ParserCache.php +++ b/includes/parser/ParserCache.php @@ -301,6 +301,10 @@ class ParserCache { $cacheTime = null, $revId = null ) { + if ( !$parserOutput->hasText() ) { + throw new InvalidArgumentException( 'Attempt to cache a ParserOutput with no text set!' ); + } + $expire = $parserOutput->getCacheExpiry(); if ( $expire > 0 && !$this->mMemc instanceof EmptyBagOStuff ) { $cacheTime = $cacheTime ?: wfTimestampNow(); diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index 7f417a29d8..78160cac5a 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -31,9 +31,9 @@ class ParserOutput extends CacheTime { const SUPPORTS_UNWRAP_TRANSFORM = 1; /** - * @var string $mText The output text + * @var string|null $mText The output text */ - public $mText; + public $mText = null; /** * @var array $mLanguageLinks List of the full text of language links, @@ -232,6 +232,15 @@ class ParserOutput extends CacheTime { const SLOW_AR_TTL = 3600; // adaptive TTL for "slow" pages const MIN_AR_TTL = 15; // min adaptive TTL (for sanity, pool counter, and edit stashing) + /** + * @param string|null $text HTML. Use null to indicate that this ParserOutput contains only + * meta-data, and the HTML output is undetermined, as opposed to empty. Passing null + * here causes hasText() to return false. + * @param array $languageLinks + * @param array $categoryLinks + * @param bool $unused + * @param string $titletext + */ public function __construct( $text = '', $languageLinks = [], $categoryLinks = [], $unused = false, $titletext = '' ) { @@ -241,6 +250,20 @@ class ParserOutput extends CacheTime { $this->mTitleText = $titletext; } + /** + * Returns true if text was passed to the constructor, or set using setText(). Returns false + * if null was passed to the $text parameter of the constructor to indicate that this + * ParserOutput only contains meta-data, and the HTML output is undetermined. + * + * @since 1.32 + * + * @return bool Whether this ParserOutput contains rendered text. If this returns false, the + * ParserOutput contains meta-data only. + */ + public function hasText() { + return ( $this->mText !== null ); + } + /** * Get the cacheable text with markers still in it. The * return value is suitable for writing back via setText() but is not valid @@ -250,6 +273,10 @@ class ParserOutput extends CacheTime { * @since 1.27 */ public function getRawText() { + if ( $this->mText === null ) { + throw new LogicException( 'This ParserOutput contains no text!' ); + } + return $this->mText; } @@ -285,7 +312,7 @@ class ParserOutput extends CacheTime { 'deduplicateStyles' => true, 'wrapperDivClass' => $this->getWrapperDivClass(), ]; - $text = $this->mText; + $text = $this->getRawText(); Hooks::runWithoutAbort( 'ParserOutputPostCacheTransform', [ $this, &$text, &$options ] ); diff --git a/tests/phpunit/includes/Revision/RenderedRevisionTest.php b/tests/phpunit/includes/Revision/RenderedRevisionTest.php index cf9dff8d9f..a2a9d09322 100644 --- a/tests/phpunit/includes/Revision/RenderedRevisionTest.php +++ b/tests/phpunit/includes/Revision/RenderedRevisionTest.php @@ -2,6 +2,7 @@ namespace MediaWiki\Tests\Revision; +use Content; use Language; use MediaWiki\Revision\RenderedRevision; use MediaWiki\Storage\MutableRevisionRecord; @@ -27,34 +28,47 @@ class RenderedRevisionTest extends MediaWikiTestCase { public function setUp() { parent::setUp(); - $this->combinerCallback = function ( RenderedRevision $rr ) { - return $this->combineOutput( $rr ); + $this->combinerCallback = function ( RenderedRevision $rr, array $hints = [] ) { + return $this->combineOutput( $rr, $hints ); }; } - private function combineOutput( RenderedRevision $rrev ) { + private function combineOutput( RenderedRevision $rrev, array $hints = [] ) { + // NOTE: the is a slightly simplified version of RevisionRenderer::combineSlotOutput + + $withHtml = $hints['generate-html'] ?? true; + $revision = $rrev->getRevision(); $slots = $revision->getSlots()->getSlots(); - $output = new ParserOutput(); - $html = ''; + $combinedOutput = new ParserOutput( null ); + $slotOutput = []; foreach ( $slots as $role => $slot ) { + $out = $rrev->getSlotParserOutput( $role, $hints ); + $slotOutput[$role] = $out; - if ( $html !== '' ) { - // skip header for the first slot - $html .= "(($role))"; - } + $combinedOutput->mergeInternalMetaDataFrom( $out ); + $combinedOutput->mergeTrackingMetaDataFrom( $out ); + } + + if ( $withHtml ) { + $html = ''; + /** @var ParserOutput $out */ + foreach ( $slotOutput as $role => $out ) { - $slotOutput = $rrev->getSlotParserOutput( $role ); - $html .= $slotOutput->getRawText(); + if ( $html !== '' ) { + // skip header for the first slot + $html .= "(($role))"; + } - $output->mergeInternalMetaDataFrom( $slotOutput, $role ); - $output->mergeHtmlMetaDataFrom( $slotOutput ); - $output->mergeTrackingMetaDataFrom( $slotOutput ); + $html .= $out->getRawText(); + $combinedOutput->mergeHtmlMetaDataFrom( $out ); + } + + $combinedOutput->setText( $html ); } - $output->setText( $html ); - return $output; + return $combinedOutput; } /** @@ -317,7 +331,7 @@ class RenderedRevisionTest extends MediaWikiTestCase { $this->assertSame( $html, $rr->getSlotParserOutput( 'main' )->getText() ); } - public function testGetRenderedRevision_multi() { + public function testGetRevisionParserOutput_multi() { $title = $this->getMockTitle( 7, 21 ); $rev = new MutableRevisionRecord( $title ); @@ -355,6 +369,40 @@ class RenderedRevisionTest extends MediaWikiTestCase { $this->assertFalse( isset( $auxLinks[NS_MAIN]['Kittens'] ), 'no main links in aux' ); } + public function testNoHtml() { + /** @var MockObject|Content $mockContent */ + $mockContent = $this->getMockBuilder( WikitextContent::class ) + ->setMethods( [ 'getParserOutput' ] ) + ->setConstructorArgs( [ 'Whatever' ] ) + ->getMock(); + $mockContent->method( 'getParserOutput' ) + ->willReturnCallback( function ( Title $title, $revId = null, + ParserOptions $options = null, $generateHtml = true + ) { + if ( !$generateHtml ) { + return new ParserOutput( null ); + } else { + $this->fail( 'Should not be called with $generateHtml == true' ); + return null; // never happens, make analyzer happy + } + } ); + + $title = $this->getMockTitle( 7, 21 ); + + $rev = new MutableRevisionRecord( $title ); + $rev->setContent( 'main', $mockContent ); + $rev->setContent( 'aux', $mockContent ); + + $options = ParserOptions::newCanonical( 'canonical' ); + $rr = new RenderedRevision( $title, $rev, $options, $this->combinerCallback ); + + $output = $rr->getSlotParserOutput( 'main', [ 'generate-html' => false ] ); + $this->assertFalse( $output->hasText(), 'hasText' ); + + $output = $rr->getRevisionParserOutput( [ 'generate-html' => false ] ); + $this->assertFalse( $output->hasText(), 'hasText' ); + } + public function testUpdateRevision() { $title = $this->getMockTitle( 7, 21 ); diff --git a/tests/phpunit/includes/Revision/RevisionRendererTest.php b/tests/phpunit/includes/Revision/RevisionRendererTest.php index fc0fa0c14e..ea195f1087 100644 --- a/tests/phpunit/includes/Revision/RevisionRendererTest.php +++ b/tests/phpunit/includes/Revision/RevisionRendererTest.php @@ -2,6 +2,7 @@ namespace MediaWiki\Tests\Revision; +use Content; use Language; use LogicException; use MediaWiki\Revision\RevisionRenderer; @@ -10,6 +11,7 @@ use MediaWiki\Storage\RevisionRecord; use MediaWiki\User\UserIdentityValue; use MediaWikiTestCase; use ParserOptions; +use ParserOutput; use PHPUnit\Framework\MockObject\MockObject; use Title; use User; @@ -420,4 +422,39 @@ class RevisionRendererTest extends MediaWikiTestCase { $this->assertFalse( isset( $auxLinks[NS_MAIN]['Kittens'] ), 'no main links in aux' ); } + public function testGetRenderedRevision_noHtml() { + /** @var MockObject|Content $mockContent */ + $mockContent = $this->getMockBuilder( WikitextContent::class ) + ->setMethods( [ 'getParserOutput' ] ) + ->setConstructorArgs( [ 'Whatever' ] ) + ->getMock(); + $mockContent->method( 'getParserOutput' ) + ->willReturnCallback( function ( Title $title, $revId = null, + ParserOptions $options = null, $generateHtml = true + ) { + if ( !$generateHtml ) { + return new ParserOutput( null ); + } else { + $this->fail( 'Should not be called with $generateHtml == true' ); + return null; // never happens, make analyzer happy + } + } ); + + $renderer = $this->newRevisionRenderer(); + $title = $this->getMockTitle( 7, 21 ); + + $rev = new MutableRevisionRecord( $title ); + $rev->setContent( 'main', $mockContent ); + $rev->setContent( 'aux', $mockContent ); + + // NOTE: we are testing the private combineSlotOutput() callback here. + $rr = $renderer->getRenderedRevision( $rev ); + + $output = $rr->getSlotParserOutput( 'main', [ 'generate-html' => false ] ); + $this->assertFalse( $output->hasText(), 'hasText' ); + + $output = $rr->getRevisionParserOutput( [ 'generate-html' => false ] ); + $this->assertFalse( $output->hasText(), 'hasText' ); + } + } diff --git a/tests/phpunit/includes/parser/ParserOutputTest.php b/tests/phpunit/includes/parser/ParserOutputTest.php index ecc4df7830..3c73430022 100644 --- a/tests/phpunit/includes/parser/ParserOutputTest.php +++ b/tests/phpunit/includes/parser/ParserOutputTest.php @@ -307,6 +307,44 @@ EOF // phpcs:enable } + /** + * @covers ParserOutput::hasText + */ + public function testHasText() { + $po = new ParserOutput(); + $this->assertTrue( $po->hasText() ); + + $po = new ParserOutput( null ); + $this->assertFalse( $po->hasText() ); + + $po = new ParserOutput( '' ); + $this->assertTrue( $po->hasText() ); + + $po = new ParserOutput( null ); + $po->setText( '' ); + $this->assertTrue( $po->hasText() ); + } + + /** + * @covers ParserOutput::getText + */ + public function testGetText_failsIfNoText() { + $po = new ParserOutput( null ); + + $this->setExpectedException( LogicException::class ); + $po->getText(); + } + + /** + * @covers ParserOutput::getRawText + */ + public function testGetRawText_failsIfNoText() { + $po = new ParserOutput( null ); + + $this->setExpectedException( LogicException::class ); + $po->getRawText(); + } + public function provideMergeHtmlMetaDataFrom() { // title text ------------ $a = new ParserOutput(); -- 2.20.1