From 89028e0b8e32c5da4b0d8a0a051fdfdbed506ad9 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 7 Oct 2021 07:45:45 -0700 Subject: [PATCH] Move limit report rendering to ParserOutput This does not move the actual limit report data into ParserOptions yet, that should be done separately given that it will require serialization changes. Let's get this change settled first before messing with serialization. This unifies canonical and non-canonical ParserOptions, so ParserCache can now be used with both. It is hard to say how this will affect the ParserCache capacity, so we should monitor it after releasing this. Change-Id: I154c0a77a5b0287b5572614d56339fb57ac56c33 --- RELEASE-NOTES-1.38 | 5 ++ includes/EditPage.php | 4 +- includes/actions/McrUndoAction.php | 6 +- includes/api/ApiFormatBase.php | 1 + includes/api/ApiHelp.php | 1 + includes/api/ApiParse.php | 2 +- includes/page/Article.php | 2 + includes/parser/Parser.php | 53 +------------ includes/parser/ParserOptions.php | 52 ++++-------- includes/parser/ParserOutput.php | 79 +++++++++++++++++++ tests/phpunit/includes/api/ApiParseTest.php | 5 +- .../includes/parser/ParserMethodsTest.php | 2 - .../includes/parser/ParserOptionsTest.php | 12 --- 13 files changed, 117 insertions(+), 107 deletions(-) diff --git a/RELEASE-NOTES-1.38 b/RELEASE-NOTES-1.38 index f457e950353..bceeffaaf47 100644 --- a/RELEASE-NOTES-1.38 +++ b/RELEASE-NOTES-1.38 @@ -212,6 +212,10 @@ 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 === @@ -305,6 +309,7 @@ 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 == diff --git a/includes/EditPage.php b/includes/EditPage.php index c0ebc2ac1d4..0e5b37570ae 100644 --- a/includes/EditPage.php +++ b/includes/EditPage.php @@ -4212,7 +4212,6 @@ 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 @@ -4250,7 +4249,8 @@ class EditPage implements IEditObject { return [ 'parserOutput' => $parserOutput, 'html' => $parserOutput->getText( [ - 'enableSectionEditLinks' => false + 'enableSectionEditLinks' => false, + 'includeLimitReport' => true, ] ) ]; } diff --git a/includes/actions/McrUndoAction.php b/includes/actions/McrUndoAction.php index 103cde89fc0..35d60e3751e 100644 --- a/includes/actions/McrUndoAction.php +++ b/includes/actions/McrUndoAction.php @@ -290,12 +290,14 @@ 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 ] ); + $previewHTML = $parserOutput->getText( [ + 'enableSectionEditLinks' => false, + 'includeLimitReport' => true, + ] ); $out->addParserOutputMetadata( $parserOutput ); if ( count( $parserOutput->getWarnings() ) ) { diff --git a/includes/api/ApiFormatBase.php b/includes/api/ApiFormatBase.php index dab7eb75877..f8634477e72 100644 --- a/includes/api/ApiFormatBase.php +++ b/includes/api/ApiFormatBase.php @@ -273,6 +273,7 @@ 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 ) diff --git a/includes/api/ApiHelp.php b/includes/api/ApiHelp.php index 65ffd856e9e..d385a8bb9f0 100644 --- a/includes/api/ApiHelp.php +++ b/includes/api/ApiHelp.php @@ -187,6 +187,7 @@ 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 ) { diff --git a/includes/api/ApiParse.php b/includes/api/ApiParse.php index d52e75a238d..18dfdb6f60e 100644 --- a/includes/api/ApiParse.php +++ b/includes/api/ApiParse.php @@ -502,6 +502,7 @@ 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 ) { @@ -717,7 +718,6 @@ 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'] ); diff --git a/includes/page/Article.php b/includes/page/Article.php index 31898768e65..55f668f7001 100644 --- a/includes/page/Article.php +++ b/includes/page/Article.php @@ -511,6 +511,8 @@ class Article implements Page { if ( $this->viewIsRenderAction ) { $poOptions += [ 'absoluteURLs' => true ]; } + $poOptions += [ 'includeLimitReport' => true ]; + $continue = $this->generateContentOutput( $user, $parserOptions, $oldid, $outputPage, $poOptions ); diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index 10660b4058a..ea501697f55 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -675,9 +675,7 @@ class Parser { $this->currentRevisionCache = null; $this->mInputSize = strlen( $text ); - if ( $this->mOptions->getEnableLimitReport() ) { - $this->mOutput->resetParseStartTime(); - } + $this->mOutput->resetParseStartTime(); $oldRevisionId = $this->mRevisionId; $oldRevisionRecordObject = $this->mRevisionRecordObject; @@ -730,8 +728,8 @@ class Parser { } # Information on limits, for the benefit of users who try to skirt them - if ( $this->mOptions->getEnableLimitReport() ) { - $text .= $this->makeLimitReport(); + if ( MediaWikiServices::getInstance()->getMainConfig()->get( 'EnableParserLimitReporting' ) ) { + $this->makeLimitReport(); } # Wrap non-interface parser output in a
so it can be targeted @@ -755,10 +753,7 @@ class Parser { } /** - * Set the limit report data in the current ParserOutput, and return the - * limit report HTML comment. - * - * @return string + * Set the limit report data in the current ParserOutput. */ protected function makeLimitReport() { $maxIncludeSize = $this->mOptions->getMaxIncludeSize(); @@ -797,42 +792,6 @@ 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( [ '-', '&' ], [ '‐', '&' ], $limitReport ); - $text = "\n\n"; - // Add on template profiling data in human/machine readable way $dataByFunc = $this->mProfiler->getFunctionStats(); uasort( $dataByFunc, static function ( $a, $b ) { @@ -844,8 +803,6 @@ class Parser { $item['%real'], $item['real'], $item['calls'], htmlspecialchars( $item['name'] ) ); } - $text .= "\n"; $this->mOutput->setLimitReportData( 'limitreport-timingprofile', $profileReport ); @@ -859,8 +816,6 @@ class Parser { $this->mOutput->getCacheExpiry() ); $this->mOutput->setLimitReportData( 'cachereport-transientcontent', $this->mOutput->hasDynamicContent() ); - - return $text; } /** diff --git a/includes/parser/ParserOptions.php b/includes/parser/ParserOptions.php index 2b1f5f10459..b49cfff8c38 100644 --- a/includes/parser/ParserOptions.php +++ b/includes/parser/ParserOptions.php @@ -476,20 +476,26 @@ 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 $this->getOption( 'enableLimitReport' ); + return false; } /** + * @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 $this->setOptionLegacy( 'enableLimitReport', $x ); + return false; } /** @@ -1006,8 +1012,6 @@ class ParserOptions { } /** - * @warning For interaction with the parser cache, use - * WikiPage::makeParserOptions() or ParserOptions::newCanonical() instead. * @param UserIdentity $user * @param Language|null $lang */ @@ -1022,8 +1026,6 @@ 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 */ @@ -1036,8 +1038,6 @@ 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,8 +1048,6 @@ 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 @@ -1061,8 +1059,6 @@ 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 */ @@ -1079,6 +1075,8 @@ 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 @@ -1100,10 +1098,6 @@ 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; } @@ -1122,9 +1116,8 @@ class ParserOptions { /** * Get default option values - * @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 + * @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 * that somehow (e.g. with the RejectParserCacheValue hook) because * MediaWiki won't do it for you. * @return array @@ -1198,23 +1191,6 @@ 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 * @@ -1382,7 +1358,7 @@ class ParserOptions { } $options = $this->options; - $defaults = self::getCanonicalOverrides() + self::getDefaults(); + $defaults = 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. @@ -1435,7 +1411,7 @@ class ParserOptions { * @since 1.30 */ public function isSafeToCache( array $usedOptions = null ) { - $defaults = self::getCanonicalOverrides() + self::getDefaults(); + $defaults = self::getDefaults(); $inCacheKey = self::getCacheVaryingOptionsHash(); $usedOptions = $usedOptions ?? array_keys( $this->options ); foreach ( $usedOptions as $option ) { diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index c0cd9bc5e05..17651c4b4f3 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -354,6 +354,7 @@ 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 */ @@ -366,9 +367,14 @@ 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'] ) { @@ -1603,6 +1609,79 @@ 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"; + + $profileReport = $limitReportData['limitreport-timingprofile'] ?? null; + if ( $profileReport ) { + $text .= "\n"; + } + + return $text; + } + /** * Check whether the cache TTL was lowered from the site default. * diff --git a/tests/phpunit/includes/api/ApiParseTest.php b/tests/phpunit/includes/api/ApiParseTest.php index 4e54c44729c..b9bf2d2425e 100644 --- a/tests/phpunit/includes/api/ApiParseTest.php +++ b/tests/phpunit/includes/api/ApiParseTest.php @@ -100,6 +100,9 @@ class ApiParseTest extends ApiTestCase { $html = substr( $html, strlen( $expectedStart ) ); + $possibleParserCache = '/\n)\n/'; + $html = preg_replace( $possibleParserCache, '', $html ); + if ( $res[1]->getBool( 'disablelimitreport' ) ) { $expectedEnd = "
"; $this->assertSame( $expectedEnd, substr( $html, -strlen( $expectedEnd ) ) ); @@ -112,7 +115,7 @@ class ApiParseTest extends ApiTestCase { } else { $expectedEnd = '#\n)\n' . ')\n' . - '(\n)\n)?$#s'; + '$#s'; $this->assertRegExp( $expectedEnd, $html ); $html = preg_replace( $expectedEnd, '', $html ); diff --git a/tests/phpunit/includes/parser/ParserMethodsTest.php b/tests/phpunit/includes/parser/ParserMethodsTest.php index 5d9b8aa3dd9..17e65b95163 100644 --- a/tests/phpunit/includes/parser/ParserMethodsTest.php +++ b/tests/phpunit/includes/parser/ParserMethodsTest.php @@ -323,8 +323,6 @@ 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' ) ); diff --git a/tests/phpunit/includes/parser/ParserOptionsTest.php b/tests/phpunit/includes/parser/ParserOptionsTest.php index d2a79d773d7..e93e0b83cb9 100644 --- a/tests/phpunit/includes/parser/ParserOptionsTest.php +++ b/tests/phpunit/includes/parser/ParserOptionsTest.php @@ -90,8 +90,6 @@ class ParserOptionsTest extends MediaWikiLangTestCase { } public static function provideIsSafeToCache() { - global $wgEnableParserLimitReporting; - $seven = static function () { return 7; }; @@ -127,12 +125,6 @@ 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, - ] ], ]; } @@ -273,10 +265,6 @@ 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 ) );