Make HTML generation in RenderedRevision optional
authordaniel <daniel.kinzler@wikimedia.de>
Mon, 13 Aug 2018 20:33:31 +0000 (22:33 +0200)
committerGergő Tisza <gtisza@wikimedia.org>
Fri, 31 Aug 2018 10:48:41 +0000 (10:48 +0000)
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
includes/Revision/RevisionRenderer.php
includes/Storage/DerivedPageDataUpdater.php
includes/parser/ParserCache.php
includes/parser/ParserOutput.php
tests/phpunit/includes/Revision/RenderedRevisionTest.php
tests/phpunit/includes/Revision/RevisionRendererTest.php
tests/phpunit/includes/parser/ParserOutputTest.php

index 380cfad..0c052d1 100644 (file)
@@ -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];
index 8f44a19..f71f9e7 100644 (file)
@@ -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;
        }
 
 }
index 736b0ca..2df1670 100644 (file)
@@ -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 ]
+               );
        }
 
        /**
index 5e6081d..43c72b1 100644 (file)
@@ -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();
index 7f417a2..78160ca 100644 (file)
@@ -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 <mw:editsection> 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 ] );
 
index cf9dff8..a2a9d09 100644 (file)
@@ -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 );
 
index fc0fa0c..ea195f1 100644 (file)
@@ -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' );
+       }
+
 }
index ecc4df7..3c73430 100644 (file)
@@ -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();