Revert "Move NewPP limit report HTML comments to JS variables" and followups
authorBartosz Dziewoński <matma.rex@gmail.com>
Tue, 8 Nov 2016 21:03:21 +0000 (22:03 +0100)
committerBartosz Dziewoński <matma.rex@gmail.com>
Tue, 8 Nov 2016 21:35:15 +0000 (22:35 +0100)
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
includes/EditPage.php
includes/OutputPage.php
includes/parser/Parser.php
includes/parser/ParserOutput.php
includes/resourceloader/ResourceLoader.php

index b5dc3f9..df9feda 100644 (file)
@@ -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).
index 9c5c91a..ad308b1 100644 (file)
@@ -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() ) .
index 76bfaa2..50629ba 100644 (file)
@@ -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;
-       }
 }
index 3f703e3..c7db8a1 100644 (file)
@@ -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( [ '-', '&' ], [ '‐', '&amp;' ], $limitReport );
-                               $text .= "\n<!-- \nNewPP limit report\n$limitReport-->\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( [ '-', '&' ], [ '‐', '&amp;' ], $limitReport );
+                       $text .= "\n<!-- \n$limitReport-->\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$profileReport-->\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() );
                        }
index 9dfa97c..d2ef5e3 100644 (file)
@@ -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;
        }
 
        /**
index c9dacbc..1073de0 100644 (file)
@@ -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()
                );
        }