From 0e15a6068a5a07fc109b5898ae51fdb8decafaf0 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Tue, 8 Nov 2016 22:03:21 +0100 Subject: [PATCH] Revert "Move NewPP limit report HTML comments to JS variables" and followups This change resulted in unreasonable feature loss (human-readable limit report was gone). Three months and multiple followups later, the functionality is still not completely restored. Given lack of response from the original author, I think it is time to revert and reconsider, especially since the 1.28 release is soon. A machine-readable limit report would be a very useful feature, but not at the cost of losing human-readable limit report. This reverts the following commits: * Move NewPP limit report HTML comments to JS variables b7c4c8717f964d1890d185ec3e6e9481fcb734e4 * Only pretty-print the parser report JS vars 28adc4d7eef2d7d8e5696a4f9849538a769daa00 * Show wgPageParseReport on page previews too 1255654ed5a89ed57491bda38f544ed87e3bc601 * Re-add human readable parser limit report 0051f108b954b52b9981d5d85862ac1f292db80c * Restore hooks.txt for ParserLimitReportFormat 4663e7a7371fabb96ed9c909e5b93042c5f08438 Resolved minor merge conflicts in OutputPage (with 80e5b160) and release notes. Bug: T110763 Bug: T142210 Change-Id: Id88c8066fae3f369e8977b4b7488f67071bdeeb7 --- RELEASE-NOTES-1.28 | 1 - includes/EditPage.php | 12 +--- includes/OutputPage.php | 27 +-------- includes/parser/Parser.php | 70 +++++++++++++--------- includes/parser/ParserOutput.php | 42 ++++++------- includes/resourceloader/ResourceLoader.php | 5 +- 6 files changed, 65 insertions(+), 92 deletions(-) diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28 index b5dc3f9b63..df9fedaf76 100644 --- a/RELEASE-NOTES-1.28 +++ b/RELEASE-NOTES-1.28 @@ -211,7 +211,6 @@ changes to languages because of Phabricator reports. * Skin::linkKnown() (use MediaWiki\Linker\LinkRenderer instead) * Skin::userLink() (use Linker::userLink() instead) * Skin::userToolLinks() (use Linker::userToolLinks() instead) -* The 'ParserLimitReportFormat' hook was removed. * Disabled "bug 2702" HTML tidying of parsed UI messages on wikis where Tidy is disabled. * DifferenceEngine::generateDiffBody() was removed (deprecated since 1.21). diff --git a/includes/EditPage.php b/includes/EditPage.php index 9c5c91ab55..ad308b1dde 100644 --- a/includes/EditPage.php +++ b/includes/EditPage.php @@ -3550,22 +3550,16 @@ HTML ] ) . Html::openElement( 'tbody' ); - foreach ( $output->getLimitReportData()['limitreport'] as $key => $value ) { + foreach ( $output->getLimitReportData() as $key => $value ) { if ( Hooks::run( 'ParserLimitReportFormat', [ $key, &$value, &$limitReport, true, true ] ) ) { - $keyMsg = wfMessage( "limitreport-$key" ); - $valueMsg = wfMessage( - [ "limitreport-$key-value-html", "limitreport-$key-value" ] - ); + $keyMsg = wfMessage( $key ); + $valueMsg = wfMessage( [ "$key-value-html", "$key-value" ] ); if ( !$valueMsg->exists() ) { $valueMsg = new RawMessage( '$1' ); } if ( !$keyMsg->isDisabled() && !$valueMsg->isDisabled() ) { - // If it's a value/limit array, convert it for $1/$2 - if ( is_array( $value ) && isset( $value['value'] ) ) { - $value = [ $value['value'], $value['limit'] ]; - } $limitReport .= Html::openElement( 'tr' ) . Html::rawElement( 'th', null, $keyMsg->parse() ) . Html::rawElement( 'td', null, $valueMsg->params( $value )->parse() ) . diff --git a/includes/OutputPage.php b/includes/OutputPage.php index 76bfaa26f9..50629ba154 100644 --- a/includes/OutputPage.php +++ b/includes/OutputPage.php @@ -295,9 +295,6 @@ class OutputPage extends ContextSource { */ private $copyrightUrl; - /** @var array Profiling data */ - private $limitReportData = []; - /** * Constructor for OutputPage. This should not be called directly. * Instead a new RequestContext should be created and it will implicitly create @@ -1775,16 +1772,11 @@ class OutputPage extends ContextSource { } } - // Enable OOUI if requested via ParserOutput + // enable OOUI if requested via ParserOutput if ( $parserOutput->getEnableOOUI() ) { $this->enableOOUI(); } - // Include profiling data - if ( !$this->limitReportData ) { - $this->setLimitReportData( $parserOutput->getLimitReportData() ); - } - // Link flags are ignored for now, but may in the future be // used to mark individual language links. $linkFlags = []; @@ -2966,15 +2958,6 @@ class OutputPage extends ContextSource { } } - if ( $this->limitReportData ) { - $chunks[] = ResourceLoader::makeInlineScript( - ResourceLoader::makeConfigSetScript( - [ 'wgPageParseReport' => $this->limitReportData ], - true - ) - ); - } - return self::combineWrappedStrings( $chunks ); } @@ -3876,12 +3859,4 @@ class OutputPage extends ContextSource { 'mediawiki.widgets.styles', ] ); } - - /** - * @param array $data Data from ParserOutput::getLimitReportData() - * @since 1.28 - */ - public function setLimitReportData( array $data ) { - $this->limitReportData = $data; - } } diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index 3f703e3cb5..c7db8a190b 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -507,46 +507,60 @@ class Parser { [ $this->mHighestExpansionDepth, $this->mOptions->getMaxPPExpandDepth() ] ); $this->mOutput->setLimitReportData( 'limitreport-expensivefunctioncount', - [ $this->mExpensiveFunctionCount, - $this->mOptions->getExpensiveParserFunctionLimit() ] + [ $this->mExpensiveFunctionCount, $this->mOptions->getExpensiveParserFunctionLimit() ] ); Hooks::run( 'ParserLimitReportPrepare', [ $this, $this->mOutput ] ); - $limitReport = ''; - Hooks::run( 'ParserLimitReport', [ $this, &$limitReport ] ); - if ( $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 = "NewPP limit report\n"; + if ( $wgShowHostnames ) { + $limitReport .= 'Parsed by ' . wfHostname() . "\n"; + } + $limitReport .= 'Cached time: ' . $this->mOutput->getCacheTime() . "\n"; + $limitReport .= 'Cache expiry: ' . $this->mOutput->getCacheExpiry() . "\n"; + $limitReport .= 'Dynamic content: ' . + ( $this->mOutput->hasDynamicContent() ? 'true' : 'false' ) . + "\n"; + + foreach ( $this->mOutput->getLimitReportData() as $key => $value ) { + if ( Hooks::run( 'ParserLimitReportFormat', + [ $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 ); + Hooks::run( 'ParserLimitReport', [ $this, &$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 + // Add on template profiling data $dataByFunc = $this->mProfiler->getFunctionStats(); uasort( $dataByFunc, function ( $a, $b ) { return $a['real'] < $b['real']; // descending order } ); - $profileReport = []; + $profileReport = "Transclusion expansion time report (%,ms,calls,template)\n"; foreach ( array_slice( $dataByFunc, 0, 10 ) as $item ) { - $profileReport[] = sprintf( "%6.2f%% %8.3f %6d %s", - $item['%real'], $item['real'], $item['calls'], $item['name'] ); + $profileReport .= sprintf( "%6.2f%% %8.3f %6d - %s\n", + $item['%real'], $item['real'], $item['calls'], + htmlspecialchars( $item['name'] ) ); } - $this->mOutput->setLimitReportData( 'limitreport-timingprofile', $profileReport ); + $text .= "\n\n"; - // Add other cache related metadata - if ( $wgShowHostnames ) { - $this->mOutput->setLimitReportData( 'cachereport-origin', wfHostname() ); - } - $this->mOutput->setLimitReportData( 'cachereport-timestamp', - $this->mOutput->getCacheTime() ); - $this->mOutput->setLimitReportData( 'cachereport-ttl', - $this->mOutput->getCacheExpiry() ); - $this->mOutput->setLimitReportData( 'cachereport-transientcontent', - $this->mOutput->hasDynamicContent() ); - - if ( $this->mGeneratedPPNodeCount - > $this->mOptions->getMaxGeneratedPPNodeCount() / 10 - ) { + if ( $this->mGeneratedPPNodeCount > $this->mOptions->getMaxGeneratedPPNodeCount() / 10 ) { wfDebugLog( 'generated-pp-node-count', $this->mGeneratedPPNodeCount . ' ' . $this->mTitle->getPrefixedDBkey() ); } diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index 9dfa97cb5a..d2ef5e3039 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -188,7 +188,9 @@ class ParserOutput extends CacheTime { */ private $mExtensionData = []; - /** @var array $mLimitReportData Parser limit report data. */ + /** + * @var array $mLimitReportData Parser limit report data. + */ private $mLimitReportData = []; /** @@ -991,34 +993,24 @@ class ParserOutput extends CacheTime { /** * Sets parser limit report data for a key * - * If $value consist of a list of two floats, it will be interpreted as - * (actual value, maximum allowed value). The presence of a "-" in $key will cause - * the first part of the key to be interpreted as a namespace. + * The key is used as the prefix for various messages used for formatting: + * - $key: The label for the field in the limit report + * - $key-value-text: Message used to format the value in the "NewPP limit + * report" HTML comment. If missing, uses $key-format. + * - $key-value-html: Message used to format the value in the preview + * limit report table. If missing, uses $key-format. + * - $key-value: Message used to format the value. If missing, uses "$1". + * + * Note that all values are interpreted as wikitext, and so should be + * encoded with htmlspecialchars() as necessary, but should avoid complex + * HTML for sanity of display in the "NewPP limit report" comment. * * @since 1.22 - * @param string $key Data key - * @param mixed $value Data value One of (float, string, bool, JSON serializable array) + * @param string $key Message key + * @param mixed $value Appropriate for Message::params() */ public function setLimitReportData( $key, $value ) { - if ( is_array( $value ) ) { - if ( array_keys( $value ) === [ 0, 1 ] - && is_numeric( $value[0] ) - && is_numeric( $value[1] ) - ) { - $data = [ 'value' => $value[0], 'limit' => $value[1] ]; - } else { - $data = $value; - } - } else { - $data = $value; - } - - if ( strpos( $key, '-' ) ) { - list( $ns, $name ) = explode( '-', $key, 2 ); - $this->mLimitReportData[$ns][$name] = $data; - } else { - $this->mLimitReportData[$key] = $data; - } + $this->mLimitReportData[$key] = $value; } /** diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index c9dacbcae9..1073de0ef6 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -1430,14 +1430,13 @@ MESSAGE; * the given value. * * @param array $configuration List of configuration values keyed by variable name - * @param bool $pretty Pretty-print with extra whitespace * @return string */ - public static function makeConfigSetScript( array $configuration, $pretty = null ) { + public static function makeConfigSetScript( array $configuration ) { return Xml::encodeJsCall( 'mw.config.set', [ $configuration ], - ( $pretty === null ) ? ResourceLoader::inDebugMode() : $pretty + ResourceLoader::inDebugMode() ); } -- 2.20.1