Revert "Move limit report rendering to ParserOutput"

This reverts commit 89028e0b8e.

Reason for revert: Temporary until we deal with T295357

Change-Id: I556de18dbf900a9bc58d5ae22d1bf194682d0840

RELEASE-NOTES-1.38

@@ -212,10 +212,6 @@ because of Phabricator reports.
 * Http::$httpEngine, deprecated since 1.34, has been removed. The only available
   HTTP engine is now Guzzle. CurlHttpRequest and PhpHttpRequest classes were
   removed.
-* Parser option enableLimitReport was deprecated. The report is now generated
-  post-parse and can be included by providing 'includeLimitReport' option
-  to ParserOutput::getText. Thus, ParserOptions::enableLimitReport and
-  ::getEnableLimitReport methods were deprecated.
 * …
 
 === Deprecations in 1.38 ===
@@ -309,7 +305,6 @@ because of Phabricator reports.
      `mw-delete-editreasons`
   The goal of these changes is to make the HTML more similar to that of
   normal page deletion.
-* ParserOptions created with ::newFrom* or ::newCanonical are now identical.
 * …
 
 == Compatibility ==

includes/EditPage.php

@@ -4212,6 +4212,7 @@ class EditPage implements IEditObject {
 		$parserOptions = $this->page->makeParserOptions( $this->context );
 		$parserOptions->setIsPreview( true );
 		$parserOptions->setIsSectionPreview( $this->section !== null && $this->section !== '' );
+		$parserOptions->enableLimitReport();
 
 		// XXX: we could call $parserOptions->setCurrentRevisionRecordCallback here to force the
 		// current revision to be null during PST, until setupFakeRevision is called on
@@ -4249,8 +4250,7 @@ class EditPage implements IEditObject {
 		return [
 			'parserOutput' => $parserOutput,
 			'html' => $parserOutput->getText( [
-				'enableSectionEditLinks' => false,
-				'includeLimitReport' => true,
+				'enableSectionEditLinks' => false
 			] )
 		];
 	}

includes/actions/McrUndoAction.php

@@ -290,14 +290,12 @@ class McrUndoAction extends FormAction {
 			$parserOptions = $this->getWikiPage()->makeParserOptions( $this->context );
 			$parserOptions->setIsPreview( true );
 			$parserOptions->setIsSectionPreview( false );
+			$parserOptions->enableLimitReport();
 
 			$parserOutput = $this->revisionRenderer
 				->getRenderedRevision( $rev, $parserOptions, $this->context->getUser() )
 				->getRevisionParserOutput();
-			$previewHTML = $parserOutput->getText( [
-				'enableSectionEditLinks' => false,
-				'includeLimitReport' => true,
-			] );
+			$previewHTML = $parserOutput->getText( [ 'enableSectionEditLinks' => false ] );
 
 			$out->addParserOutputMetadata( $parserOutput );
 			if ( count( $parserOutput->getWarnings() ) ) {

includes/api/ApiFormatBase.php

@@ -273,7 +273,6 @@ abstract class ApiFormatBase extends ApiBase {
 				}
 
 				$header = $msg->parseAsBlock();
-				// @phan-suppress-next-line SecurityCheck-XSS
 				$out->addHTML(
 					Html::rawElement( 'div', [ 'class' => 'api-pretty-header' ],
 						ApiHelp::fixHelpLinks( $header )

includes/api/ApiHelp.php

@@ -187,7 +187,6 @@ class ApiHelp extends ApiBase {
 		$helptitle = $options['helptitle'] ?? null;
 		$html = self::fixHelpLinks( $out->getHTML(), $helptitle, $haveModules );
 		$out->clearHTML();
-		// @phan-suppress-next-line SecurityCheck-XSS
 		$out->addHTML( $html );
 
 		if ( $cacheKey !== null ) {

includes/api/ApiParse.php

@@ -502,7 +502,6 @@ class ApiParse extends ApiBase {
 				'wrapperDivClass' => $params['wrapoutputclass'],
 				'deduplicateStyles' => !$params['disablestylededuplication'],
 				'skin' => $skin,
-				'includeLimitReport' => !$params['disablepp'] && !$params['disablelimitreport']
 			] );
 			$result_array[ApiResult::META_BC_SUBELEMENTS][] = 'text';
 			if ( $context ) {
@@ -718,6 +717,7 @@ class ApiParse extends ApiBase {
 	 * @return array [ ParserOptions, ScopedCallback, bool $suppressCache ]
 	 */
 	private function tweakParserOptions( ParserOptions $popts, Title $title, array $params ) {
+		$popts->enableLimitReport( !$params['disablepp'] && !$params['disablelimitreport'] );
 		$popts->setIsPreview( $params['preview'] || $params['sectionpreview'] );
 		$popts->setIsSectionPreview( $params['sectionpreview'] );
 

includes/page/Article.php

@@ -511,8 +511,6 @@ class Article implements Page {
 		if ( $this->viewIsRenderAction ) {
 			$poOptions += [ 'absoluteURLs' => true ];
 		}
-		$poOptions += [ 'includeLimitReport' => true ];
-
 		$continue =
 			$this->generateContentOutput( $user, $parserOptions, $oldid, $outputPage, $poOptions );
 

includes/parser/Parser.php

@@ -675,7 +675,9 @@ class Parser {
 
 		$this->currentRevisionCache = null;
 		$this->mInputSize = strlen( $text );
-		$this->mOutput->resetParseStartTime();
+		if ( $this->mOptions->getEnableLimitReport() ) {
+			$this->mOutput->resetParseStartTime();
+		}
 
 		$oldRevisionId = $this->mRevisionId;
 		$oldRevisionRecordObject = $this->mRevisionRecordObject;
@@ -728,8 +730,8 @@ class Parser {
 		}
 
 		# Information on limits, for the benefit of users who try to skirt them
-		if ( MediaWikiServices::getInstance()->getMainConfig()->get( 'EnableParserLimitReporting' ) ) {
-			$this->makeLimitReport();
+		if ( $this->mOptions->getEnableLimitReport() ) {
+			$text .= $this->makeLimitReport();
 		}
 
 		# Wrap non-interface parser output in a <div> so it can be targeted
@@ -753,7 +755,10 @@ class Parser {
 	}
 
 	/**
-	 * Set the limit report data in the current ParserOutput.
+	 * Set the limit report data in the current ParserOutput, and return the
+	 * limit report HTML comment.
+	 *
+	 * @return string
 	 */
 	protected function makeLimitReport() {
 		$maxIncludeSize = $this->mOptions->getMaxIncludeSize();
@@ -792,6 +797,42 @@ class Parser {
 
 		$this->hookRunner->onParserLimitReportPrepare( $this, $this->mOutput );
 
+		$limitReport = "NewPP limit report\n";
+		if ( $this->svcOptions->get( 'ShowHostnames' ) ) {
+			$limitReport .= 'Parsed by ' . wfHostname() . "\n";
+		}
+		$limitReport .= 'Cached time: ' . $this->mOutput->getCacheTime() . "\n";
+		$limitReport .= 'Cache expiry: ' . $this->mOutput->getCacheExpiry() . "\n";
+		$limitReport .= 'Reduced expiry: ' .
+			( $this->mOutput->hasReducedExpiry() ? 'true' : 'false' ) .
+			"\n";
+		$limitReport .= 'Complications: [' . implode( ', ', $this->mOutput->getAllFlags() ) . "]\n";
+
+		foreach ( $this->mOutput->getLimitReportData() as $key => $value ) {
+			if ( $this->hookRunner->onParserLimitReportFormat(
+				$key, $value, $limitReport, false, false )
+			) {
+				$keyMsg = wfMessage( $key )->inLanguage( 'en' )->useDatabase( false );
+				$valueMsg = wfMessage( [ "$key-value-text", "$key-value" ] )
+					->inLanguage( 'en' )->useDatabase( false );
+				if ( !$valueMsg->exists() ) {
+					$valueMsg = new RawMessage( '$1' );
+				}
+				if ( !$keyMsg->isDisabled() && !$valueMsg->isDisabled() ) {
+					$valueMsg->params( $value );
+					$limitReport .= "{$keyMsg->text()}: {$valueMsg->text()}\n";
+				}
+			}
+		}
+		// Since we're not really outputting HTML, decode the entities and
+		// then re-encode the things that need hiding inside HTML comments.
+		$limitReport = htmlspecialchars_decode( $limitReport );
+
+		// Sanitize for comment. Note '‐' in the replacement is U+2010,
+		// which looks much like the problematic '-'.
+		$limitReport = str_replace( [ '-', '&' ], [ '‐', '&amp;' ], $limitReport );
+		$text = "\n<!-- \n$limitReport-->\n";
+
 		// Add on template profiling data in human/machine readable way
 		$dataByFunc = $this->mProfiler->getFunctionStats();
 		uasort( $dataByFunc, static function ( $a, $b ) {
@@ -803,6 +844,8 @@ class Parser {
 				$item['%real'], $item['real'], $item['calls'],
 				htmlspecialchars( $item['name'] ) );
 		}
+		$text .= "<!--\nTransclusion expansion time report (%,ms,calls,template)\n";
+		$text .= implode( "\n", $profileReport ) . "\n-->\n";
 
 		$this->mOutput->setLimitReportData( 'limitreport-timingprofile', $profileReport );
 
@@ -816,6 +859,8 @@ class Parser {
 			$this->mOutput->getCacheExpiry() );
 		$this->mOutput->setLimitReportData( 'cachereport-transientcontent',
 			$this->mOutput->hasDynamicContent() );
+
+		return $text;
 	}
 
 	/**

includes/parser/ParserOptions.php

@@ -476,26 +476,20 @@ class ParserOptions {
 	}
 
 	/**
-	 * @deprecated since 1.38. This does nothing now, to control limit reporting
-	 * please provide 'includeLimitReport' option to ParserOutput::getText.
-	 *
 	 * Enable limit report in an HTML comment on output
 	 * @return bool
 	 */
 	public function getEnableLimitReport() {
-		return false;
+		return $this->getOption( 'enableLimitReport' );
 	}
 
 	/**
-	 * @deprecated since 1.38. This does nothing now, to control limit reporting
-	 * please provide 'includeLimitReport' option to ParserOutput::getText.
-	 *
 	 * Enable limit report in an HTML comment on output
 	 * @param bool|null $x New value (null is no change)
 	 * @return bool Old value
 	 */
 	public function enableLimitReport( $x = true ) {
-		return false;
+		return $this->setOptionLegacy( 'enableLimitReport', $x );
 	}
 
 	/**
@@ -1012,6 +1006,8 @@ class ParserOptions {
 	}
 
 	/**
+	 * @warning For interaction with the parser cache, use
+	 *  WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead.
 	 * @param UserIdentity $user
 	 * @param Language|null $lang
 	 */
@@ -1026,6 +1022,8 @@ class ParserOptions {
 
 	/**
 	 * Get a ParserOptions object for an anonymous user
+	 * @warning For interaction with the parser cache, use
+	 *  WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead.
 	 * @since 1.27
 	 * @return ParserOptions
 	 */
@@ -1038,6 +1036,8 @@ class ParserOptions {
 	 * Get a ParserOptions object from a given user.
 	 * Language will be taken from $wgLang.
 	 *
+	 * @warning For interaction with the parser cache, use
+	 *  WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead.
 	 * @param UserIdentity $user
 	 * @return ParserOptions
 	 */
@@ -1048,6 +1048,8 @@ class ParserOptions {
 	/**
 	 * Get a ParserOptions object from a given user and language
 	 *
+	 * @warning For interaction with the parser cache, use
+	 *  WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead.
 	 * @param UserIdentity $user
 	 * @param Language $lang
 	 * @return ParserOptions
@@ -1059,6 +1061,8 @@ class ParserOptions {
 	/**
 	 * Get a ParserOptions object from a IContextSource object
 	 *
+	 * @warning For interaction with the parser cache, use
+	 *  WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead.
 	 * @param IContextSource $context
 	 * @return ParserOptions
 	 */
@@ -1075,8 +1079,6 @@ class ParserOptions {
 	 * @since 1.30
 	 * @since 1.32 Added string and IContextSource as options for the first parameter
 	 * @since 1.36 UserIdentity is also allowed
-	 * @deprecated since 1.38. Use ::newFromContext, ::newFromAnon or ::newFromUserAndLang instead.
-	 *   Canonical ParserOptions are now exactly the same as non-canonical.
 	 * @param IContextSource|string|UserIdentity $context
 	 *  - If an IContextSource, the options are initialized based on the source's UserIdentity and Language.
 	 *  - If the string 'canonical', the options are initialized with an anonymous user and
@@ -1098,6 +1100,10 @@ class ParserOptions {
 				'$context must be an IContextSource, the string "canonical", or a UserIdentity'
 			);
 		}
+
+		foreach ( self::getCanonicalOverrides() as $k => $v ) {
+			$ret->setOption( $k, $v );
+		}
 		return $ret;
 	}
 
@@ -1116,8 +1122,9 @@ class ParserOptions {
 
 	/**
 	 * Get default option values
-	 * @warning If you change the default for an existing option, all existing
-	 *  parser cache entries will be invalid. To avoid bugs, you'll need to handle
+	 * @warning If you change the default for an existing option (unless it's
+	 *  being overridden by self::getCanonicalOverrides()), all existing parser
+	 *  cache entries will be invalid. To avoid bugs, you'll need to handle
 	 *  that somehow (e.g. with the RejectParserCacheValue hook) because
 	 *  MediaWiki won't do it for you.
 	 * @return array
@@ -1191,6 +1198,23 @@ class ParserOptions {
 		];
 	}
 
+	/**
+	 * Get "canonical" non-default option values
+	 * @see self::newCanonical
+	 * @warning If you change the override for an existing option, all existing
+	 *  parser cache entries will be invalid. To avoid bugs, you'll need to
+	 *  handle that somehow (e.g. with the RejectParserCacheValue hook) because
+	 *  MediaWiki won't do it for you.
+	 * @return array
+	 */
+	private static function getCanonicalOverrides() {
+		global $wgEnableParserLimitReporting;
+
+		return [
+			'enableLimitReport' => $wgEnableParserLimitReporting,
+		];
+	}
+
 	/**
 	 * Get user options
 	 *
@@ -1358,7 +1382,7 @@ class ParserOptions {
 		}
 
 		$options = $this->options;
-		$defaults = self::getDefaults();
+		$defaults = self::getCanonicalOverrides() + self::getDefaults();
 
 		// We only include used options with non-canonical values in the key
 		// so adding a new option doesn't invalidate the entire parser cache.
@@ -1411,7 +1435,7 @@ class ParserOptions {
 	 * @since 1.30
 	 */
 	public function isSafeToCache( array $usedOptions = null ) {
-		$defaults = self::getDefaults();
+		$defaults = self::getCanonicalOverrides() + self::getDefaults();
 		$inCacheKey = self::getCacheVaryingOptionsHash();
 		$usedOptions = $usedOptions ?? array_keys( $this->options );
 		foreach ( $usedOptions as $option ) {

includes/parser/ParserOutput.php

@@ -354,7 +354,6 @@ class ParserOutput extends CacheTime {
 	 *    the scheme-specific-part of the href is the (percent-encoded) value
 	 *    of the `data-mw-deduplicate` attribute.
 	 *  - absoluteURLs: (bool) use absolute URLs in all links. Default: false
-	 *  - includeLimitReport: (bool) render PP limit report in HTML. Default: false
 	 * @return string HTML
 	 * @return-taint escaped
 	 */
@@ -367,14 +366,9 @@ class ParserOutput extends CacheTime {
 			'unwrap' => false,
 			'deduplicateStyles' => true,
 			'wrapperDivClass' => $this->getWrapperDivClass(),
-			'includeLimitReport' => false,
 		];
 		$text = $this->getRawText();
 
-		if ( $options['includeLimitReport'] ) {
-			$text .= $this->renderLimitReport();
-		}
-
 		Hooks::runner()->onParserOutputPostCacheTransform( $this, $text, $options );
 
 		if ( $options['wrapperDivClass'] !== '' && !$options['unwrap'] ) {
@@ -1609,79 +1603,6 @@ class ParserOutput extends CacheTime {
 		}
 	}
 
-	private function renderLimitReport(): string {
-		$limitReportData = $this->getLimitReportData();
-		// If nothing set it, we can't get it.
-		if ( !$limitReportData ) {
-			return '';
-		}
-
-		$limitReport = "NewPP limit report\n";
-
-		if ( array_key_exists( 'cachereport-origin', $limitReportData ) ) {
-			$limitReport .= "Parsed by {$limitReportData['cachereport-origin']}\n";
-		}
-
-		if ( array_key_exists( 'cachereport-timestamp', $limitReportData ) ) {
-			$limitReport .= "Cached time: {$limitReportData['cachereport-timestamp']}\n";
-		}
-
-		if ( array_key_exists( 'cachereport-ttl', $limitReportData ) ) {
-			$limitReport .= "Cache expiry: {$limitReportData['cachereport-ttl']}\n";
-		}
-
-		if ( array_key_exists( 'cachereport-transientcontent', $limitReportData ) ) {
-			$transient = $limitReportData['cachereport-transientcontent'] ? 'true' : 'false';
-			$limitReport .= "Reduced expiry: $transient\n";
-		}
-
-		// TODO: flags should go into limit report too.
-		$limitReport .= 'Complications: [' . implode( ', ', $this->getAllFlags() ) . "]\n";
-
-		foreach ( $limitReportData as $key => $value ) {
-			if ( in_array( $key, [
-				'cachereport-origin',
-				'cachereport-timestamp',
-				'cachereport-ttl',
-				'cachereport-transientcontent',
-				'limitreport-timingprofile'
-			] ) ) {
-				// These keys are processed separately.
-				continue;
-			}
-			if ( Hooks::runner()->onParserLimitReportFormat(
-				$key, $value, $limitReport, false, false )
-			) {
-				$keyMsg = wfMessage( $key )->inLanguage( 'en' )->useDatabase( false );
-				$valueMsg = wfMessage( [ "$key-value-text", "$key-value" ] )
-					->inLanguage( 'en' )->useDatabase( false );
-				if ( !$valueMsg->exists() ) {
-					$valueMsg = new RawMessage( '$1' );
-				}
-				if ( !$keyMsg->isDisabled() && !$valueMsg->isDisabled() ) {
-					$valueMsg->params( $value );
-					$limitReport .= "{$keyMsg->text()}: {$valueMsg->text()}\n";
-				}
-			}
-		}
-		// Since we're not really outputting HTML, decode the entities and
-		// then re-encode the things that need hiding inside HTML comments.
-		$limitReport = htmlspecialchars_decode( $limitReport );
-
-		// Sanitize for comment. Note '‐' in the replacement is U+2010,
-		// which looks much like the problematic '-'.
-		$limitReport = str_replace( [ '-', '&' ], [ '‐', '&' ], $limitReport );
-		$text = "\n<!-- \n$limitReport-->\n";
-
-		$profileReport = $limitReportData['limitreport-timingprofile'] ?? null;
-		if ( $profileReport ) {
-			$text .= "<!--\nTransclusion expansion time report (%,ms,calls,template)\n";
-			$text .= implode( "\n", $profileReport ) . "\n-->\n";
-		}
-
-		return $text;
-	}
-
 	/**
 	 * Check whether the cache TTL was lowered from the site default.
 	 *

tests/phpunit/includes/api/ApiParseTest.php

@@ -100,9 +100,6 @@ class ApiParseTest extends ApiTestCase {
 
 		$html = substr( $html, strlen( $expectedStart ) );
 
-		$possibleParserCache = '/\n<!-- Saved in (?>parser cache|RevisionOutputCache) (?>.*?\n -->)\n/';
-		$html = preg_replace( $possibleParserCache, '', $html );
-
 		if ( $res[1]->getBool( 'disablelimitreport' ) ) {
 			$expectedEnd = "</div>";
 			$this->assertSame( $expectedEnd, substr( $html, -strlen( $expectedEnd ) ) );
@@ -115,7 +112,7 @@ class ApiParseTest extends ApiTestCase {
 		} else {
 			$expectedEnd = '#\n<!-- \nNewPP limit report\n(?>.+?\n-->)\n' .
 				'<!--\nTransclusion expansion time report \(%,ms,calls,template\)\n(?>.*?\n-->)\n' .
-				'</div>$#s';
+				'(\n<!-- Saved in (?>parser cache|RevisionOutputCache) (?>.*?\n -->)\n)?</div>$#s';
 			$this->assertRegExp( $expectedEnd, $html );
 
 			$html = preg_replace( $expectedEnd, '', $html );

tests/phpunit/includes/parser/ParserMethodsTest.php

@@ -323,6 +323,8 @@ class ParserMethodsTest extends MediaWikiLangTestCase {
 			'language' => MediaWikiServices::getInstance()->getLanguageFactory()->getLanguage( 'en' )
 		] );
 
+		$po->enableLimitReport( false );
+
 		$oldRevision = new MutableRevisionRecord( $title );
 		$oldRevision->setId( 100 );
 		$oldRevision->setUser( new UserIdentityValue( 7, 'OldAuthor' ) );

tests/phpunit/includes/parser/ParserOptionsTest.php

@@ -90,6 +90,8 @@ class ParserOptionsTest extends MediaWikiLangTestCase {
 	}
 
 	public static function provideIsSafeToCache() {
+		global $wgEnableParserLimitReporting;
+
 		$seven = static function () {
 			return 7;
 		};
@@ -125,6 +127,12 @@ class ParserOptionsTest extends MediaWikiLangTestCase {
 			'Callback not default' => [ true, [
 				'speculativeRevIdCallback' => $seven,
 			] ],
+			'Canonical override, not default (1)' => [ true, [
+				'enableLimitReport' => $wgEnableParserLimitReporting,
+			] ],
+			'Canonical override, not default (2)' => [ false, [
+				'enableLimitReport' => !$wgEnableParserLimitReporting,
+			] ],
 		];
 	}
 
@@ -265,6 +273,10 @@ class ParserOptionsTest extends MediaWikiLangTestCase {
 		$popt2 = ParserOptions::newCanonical( 'canonical' );
 		$this->assertTrue( $popt1->matches( $popt2 ) );
 
+		$popt1->enableLimitReport( true );
+		$popt2->enableLimitReport( false );
+		$this->assertTrue( $popt1->matches( $popt2 ) );
+
 		$popt2->setInterfaceMessage( !$popt2->getInterfaceMessage() );
 		$this->assertFalse( $popt1->matches( $popt2 ) );