Clarify generate-html and make ParserOutput behave as expected

Previously:
* It was unclear that generate-html is an optional optimization
* Most of MediaWiki core was doing $parserOutput->setText('') if
html wasn't generated. However this is wrong and will cause
$parserOutput->hasText() to return true and also potentially cause
cache pollution if a content handler both does that and supports
parser cache (Like MassMessage; see T299896)
* The default value of mText in the constructor was '', and most
of the time MW used that default. This doesn't seem right. If
setText() is never called, the ParserOutput should not be considered
to have text
* It was impossible to set mText to null, as $parserOutput->setText(null)
was a no-op. Docs implied you were supposed to do this, so it was very
confusing.

This patch clarifies docs, changes the default value for ParserOutput::$mText
from '' to null, and makes $parserOutput->setText(null) do what you
expect it to. The last two are arguably breaking changes, although
the previous behaviours were unexpected, mostly undocumented and
based on a code search do not appear to be relied on.

It seems like the main reason this only broke MassMessage is most
content handlers either don't support generateHtml, or they don't
support parser cache.

Bug: T306591
Change-Id: I49cdf21411c6b02ac9a221a13393bebe17c7871e
Depends-On: I68ad491735b2df13951399312a4f9c37b63a08fa

RELEASE-NOTES-1.39

@@ -154,6 +154,10 @@ because of Phabricator reports.
   since 1.36, now throws an exception.
 * IResultWrapper::next() now returns void, to match the Iterator interface that
   it implements. fetchObject() has the same behavior as next() used to.
+* ParserOutput::setText will now set the ParserOutput's text to null if
+  given null. Previously it did nothing if given null.
+* The default value for the first argument to the ParserOutput constructor
+  is now null instead of ''.
 * …
 
 === Deprecations in 1.39 ===

includes/content/ContentHandler.php

@@ -1753,6 +1753,12 @@ abstract class ContentHandler {
 	 * Unless $cpoParams->getGenerateHtml() was false,
 	 * this includes an HTML representation of the content.
 	 *
+	 * If $cpoParams->getGenerateHtml() is false, and you chose not to generate
+	 * html, the ParserOutput must have a text of null. If the
+	 * text of the ParserOutput object is anything other than null (even if ''),
+	 * it is assumed that you don't support not generating html, and that it is
+	 * safe to reuse the parser output for calls expecting that html was generated.
+	 *
 	 * Subclasses are expected to override this method.
 	 *
 	 * This placeholder implementation always throws an exception.

includes/content/CssContentHandler.php

@@ -134,7 +134,7 @@ class CssContentHandler extends CodeContentHandler {
 				"\n" . $content->getText() . "\n"
 			) . "\n";
 		} else {
-			$html = '';
+			$html = null;
 		}
 
 		$output->clearWrapperDivClass();

includes/content/JavaScriptContentHandler.php

@@ -149,7 +149,7 @@ class JavaScriptContentHandler extends CodeContentHandler {
 				"\n" . $content->getText() . "\n"
 			) . "\n";
 		} else {
-			$html = '';
+			$html = null;
 		}
 
 		$output->clearWrapperDivClass();

includes/content/JsonContentHandler.php

@@ -103,7 +103,7 @@ class JsonContentHandler extends CodeContentHandler {
 			$parserOutput->setText( $content->rootValueTable( $content->getData()->getValue() ) );
 			$parserOutput->addModuleStyles( [ 'mediawiki.content.json' ] );
 		} else {
-			$parserOutput->setText( '' );
+			$parserOutput->setText( null );
 		}
 	}
 }

includes/content/TextContentHandler.php

@@ -241,7 +241,7 @@ class TextContentHandler extends ContentHandler {
 				$html = htmlspecialchars( $content->getText(), ENT_COMPAT );
 			}
 		} else {
-			$html = '';
+			$html = null;
 		}
 
 		$output->clearWrapperDivClass();

includes/content/WikitextContentHandler.php

@@ -305,6 +305,8 @@ class WikitextContentHandler extends TextContentHandler {
 					$parserOutput->getRawText()
 				);
 				$parserOutput->addModuleStyles( [ 'mediawiki.action.view.redirectPage' ] );
+			} else {
+				$parserOutput->setText( null );
 			}
 		}
 

includes/parser/ParserOutput.php

@@ -320,13 +320,14 @@ class ParserOutput extends CacheTime implements ContentMetadataCollector {
 	/**
 	 * @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.
+	 *        here causes hasText() to return false. In 1.39 the default value changed from ''
+	 *        to null.
 	 * @param array $languageLinks
 	 * @param array $categoryLinks
 	 * @param bool $unused
 	 * @param string $titletext
 	 */
-	public function __construct( $text = '', $languageLinks = [], $categoryLinks = [],
+	public function __construct( $text = null, $languageLinks = [], $categoryLinks = [],
 		$unused = false, $titletext = ''
 	) {
 		$this->mText = $text;
@@ -873,8 +874,17 @@ class ParserOutput extends CacheTime implements ContentMetadataCollector {
 		return $this->mExtraStyleSrcs;
 	}
 
+	/**
+	 * Set the text of the ParserOutput.
+	 *
+	 * If you did not generate html, pass null to mark it as such.
+	 *
+	 * @since 1.39 You can now pass null to this function
+	 * @param string|null $text HTML content of ParserOutput or null if not generated
+	 * @return string|null Previous value of ParserOutput's text
+	 */
 	public function setText( $text ) {
-		return wfSetVar( $this->mText, $text );
+		return wfSetVar( $this->mText, $text, true );
 	}
 
 	public function setLanguageLinks( $ll ) {

tests/phpunit/includes/content/WikitextContentHandlerTest.php

@@ -288,7 +288,7 @@ class WikitextContentHandlerTest extends MediaWikiLangTestCase {
 			->method( 'getDataForSearchIndex' )
 			->willReturn( [ 'file_text' => 'This is file content' ] );
 
-		$data = $handler->getDataForSearchIndex( $page, new ParserOutput(), $mockEngine );
+		$data = $handler->getDataForSearchIndex( $page, new ParserOutput( '' ), $mockEngine );
 		$this->assertArrayHasKey( 'file_text', $data );
 		$this->assertEquals( 'This is file content', $data['file_text'] );
 	}

tests/phpunit/includes/parser/ParserCacheSerializationTestCases.php

@@ -154,27 +154,27 @@ abstract class ParserCacheSerializationTestCases {
 		$parserOutputWithUsedOptions->recordOption( 'optA' );
 		$parserOutputWithUsedOptions->recordOption( 'optX' );
 
-		$parserOutputWithExtensionData = new ParserOutput();
+		$parserOutputWithExtensionData = new ParserOutput( '' );
 		foreach ( self::MOCK_EXT_DATA as $key => $value ) {
 			$parserOutputWithExtensionData->setExtensionData( $key, $value );
 		}
 
-		$parserOutputWithProperties = new ParserOutput();
+		$parserOutputWithProperties = new ParserOutput( '' );
 		foreach ( self::MOCK_EXT_DATA as $key => $value ) {
 			$parserOutputWithProperties->setPageProperty( $key, $value );
 		}
 
-		$parserOutputWithFalsyProperties = new ParserOutput();
+		$parserOutputWithFalsyProperties = new ParserOutput( '' );
 		foreach ( self::MOCK_FALSY_PROPERTIES as $key => $value ) {
 			$parserOutputWithFalsyProperties->setPageProperty( $key, $value );
 		}
 
-		$parserOutputWithBinaryProperties = new ParserOutput();
+		$parserOutputWithBinaryProperties = new ParserOutput( '' );
 		foreach ( self::MOCK_BINARY_PROPERTIES as $key => $value ) {
 			$parserOutputWithBinaryProperties->setPageProperty( $key, $value );
 		}
 
-		$parserOutputWithMetadata = new ParserOutput();
+		$parserOutputWithMetadata = new ParserOutput( '' );
 		$parserOutputWithMetadata->setSpeculativeRevIdUsed( 42 );
 		$parserOutputWithMetadata->addLanguageLink( 'link1' );
 		$parserOutputWithMetadata->addLanguageLink( 'link2' );
@@ -213,7 +213,7 @@ abstract class ParserCacheSerializationTestCases {
 		$parserOutputWithMetadata->setNewSection( true );
 		$parserOutputWithMetadata->setFlag( 'test' );
 
-		$parserOutputWithMetadataPost1_31 = new ParserOutput();
+		$parserOutputWithMetadataPost1_31 = new ParserOutput( '' );
 		$parserOutputWithMetadataPost1_31->addWrapperDivClass( 'test_wrapper' );
 		$parserOutputWithMetadataPost1_31->setSpeculativePageIdUsed( 4242 );
 		$parserOutputWithMetadataPost1_31->setRevisionTimestampUsed(
@@ -222,7 +222,7 @@ abstract class ParserCacheSerializationTestCases {
 		$parserOutputWithMetadataPost1_31->setRevisionUsedSha1Base36( 'test_hash' );
 		$parserOutputWithMetadataPost1_31->setNoGallery( true );
 
-		$parserOutputWithMetadataPost1_34 = new ParserOutput();
+		$parserOutputWithMetadataPost1_34 = new ParserOutput( '' );
 		$parserOutputWithMetadataPost1_34->addExtraCSPStyleSrc( 'style1' );
 		$parserOutputWithMetadataPost1_34->addExtraCSPDefaultSrc( 'default1' );
 		$parserOutputWithMetadataPost1_34->addExtraCSPScriptSrc( 'script1' );
@@ -230,7 +230,7 @@ abstract class ParserCacheSerializationTestCases {
 
 		return [
 			'empty' => [
-				'instance' => new ParserOutput(),
+				'instance' => new ParserOutput( '' ),
 				'assertions' => function ( MediaWikiIntegrationTestCase $testCase, ParserOutput $object ) {
 					// Empty CacheTime assertions
 					self::getCacheTimeTestCases()['empty']['assertions']( $testCase, $object );

tests/phpunit/includes/parser/ParserOutputTest.php

@@ -518,18 +518,25 @@ EOF
 	 * @covers ParserOutput::hasText
 	 */
 	public function testHasText() {
-		$po = new ParserOutput();
+		$po = new ParserOutput( '' );
 		$this->assertTrue( $po->hasText() );
 
 		$po = new ParserOutput( null );
 		$this->assertFalse( $po->hasText() );
 
+		$po = new ParserOutput();
+		$this->assertFalse( $po->hasText() );
+
 		$po = new ParserOutput( '' );
 		$this->assertTrue( $po->hasText() );
 
 		$po = new ParserOutput( null );
 		$po->setText( '' );
 		$this->assertTrue( $po->hasText() );
+
+		$po = new ParserOutput( 'foo' );
+		$po->setText( null );
+		$this->assertFalse( $po->hasText() );
 	}
 
 	/**