From 0dc7ba02b4483700a2fc3ab6bf76a3dd072ef054 Mon Sep 17 00:00:00 2001
From: daniel <daniel.kinzler@wikimedia.de>
Date: Tue, 28 Aug 2018 18:48:10 +0200
Subject: [PATCH] Apply content wrapping in ParserOutput::getText()

Instead of applying wrapping the the parser and unwrapping in
ParserOutput::getText(), turn this around and apply wrapping in getText(),
and only if desired.

This avoids search&replace logic for unwrapping, and it also makes it a lot
easier to merge the output of multiple slots for MCR output.

This changes behavior in two hopefully irrelevant ways:
1) the limit report comments will be inside the wrapper div, instead of
following it.
2) if HTML with a wrapper div is explicitly injected into a ParserOutput
object, it will not be possible to unwrap the text.

Bug: T174035
Change-Id: I1641b7995af9bd297f1acd610d583fbf874f34e0
---
 includes/api/ApiParse.php                     |   2 +-
 includes/content/TextContent.php              |   1 +
 includes/parser/Parser.php                    |   2 +-
 includes/parser/ParserOutput.php              |  60 +++++++---
 tests/phpunit/includes/api/ApiParseTest.php   |   6 +-
 .../includes/parser/ParserMethodsTest.php     |  12 ++
 .../includes/parser/ParserOutputTest.php      | 107 ++++++++++--------
 7 files changed, 123 insertions(+), 67 deletions(-)

diff --git a/includes/api/ApiParse.php b/includes/api/ApiParse.php
index 3a604715bc..5c25b5a0e5 100644
--- a/includes/api/ApiParse.php
+++ b/includes/api/ApiParse.php
@@ -341,7 +341,7 @@ class ApiParse extends ApiBase {
 			$result_array['text'] = $p_result->getText( [
 				'allowTOC' => !$params['disabletoc'],
 				'enableSectionEditLinks' => !$params['disableeditsection'],
-				'unwrap' => $params['wrapoutputclass'] === '',
+				'wrapperDivClass' => $params['wrapoutputclass'],
 				'deduplicateStyles' => !$params['disablestylededuplication'],
 			] );
 			$result_array[ApiResult::META_BC_SUBELEMENTS][] = 'text';
diff --git a/includes/content/TextContent.php b/includes/content/TextContent.php
index 20bce3701a..0198a0de11 100644
--- a/includes/content/TextContent.php
+++ b/includes/content/TextContent.php
@@ -253,6 +253,7 @@ class TextContent extends AbstractContent {
 			$html = '';
 		}
 
+		$output->clearWrapperDivClass();
 		$output->setText( $html );
 	}
 
diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
index 6bee1692c4..47f81e612a 100644
--- a/includes/parser/Parser.php
+++ b/includes/parser/Parser.php
@@ -517,7 +517,7 @@ class Parser {
 		# with CSS (T37247)
 		$class = $this->mOptions->getWrapOutputClass();
 		if ( $class !== false && !$this->mOptions->getInterfaceMessage() ) {
-			$text = Html::rawElement( 'div', [ 'class' => $class ], $text );
+			$this->mOutput->addWrapperDivClass( $class );
 		}
 
 		$this->mOutput->setText( $text );
diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php
index 182648a94f..fe9913d174 100644
--- a/includes/parser/ParserOutput.php
+++ b/includes/parser/ParserOutput.php
@@ -212,6 +212,11 @@ class ParserOutput extends CacheTime {
 	/** @var int|null Assumed rev ID for {{REVISIONID}} if no revision is set */
 	private $mSpeculativeRevId;
 
+	/** string CSS classes to use for the wrapping div, stored in the array keys.
+	 * If no class is given, no wrapper is added.
+	 */
+	private $mWrapperDivClasses = [];
+
 	/** @var int Upper bound of expiry based on parse duration */
 	private $mMaxAdaptiveExpiry = INF;
 
@@ -258,7 +263,12 @@ class ParserOutput extends CacheTime {
 	 *  - enableSectionEditLinks: (bool) Include section edit links, assuming
 	 *     section edit link tokens are present in the HTML. Default is true,
 	 *     but might be statefully overridden.
-	 *  - unwrap: (bool) Remove a wrapping mw-parser-output div. Default is false.
+	 *  - unwrap: (bool) Return text without a wrapper div. Default is false,
+	 *    meaning a wrapper div will be added if getWrapperDivClass() returns
+	 *    a non-empty string.
+	 *  - wrapperDivClass: (string) Wrap the output in a div and apply the given
+	 *    CSS class to that div. This overrides the output of getWrapperDivClass().
+	 *    Setting this to an empty string has the same effect as 'unwrap' => true.
 	 *  - deduplicateStyles: (bool) When true, which is the default, `<style>`
 	 *    tags with the `data-mw-deduplicate` attribute set are deduplicated by
 	 *    value of the attribute: all but the first will be replaced by `<link
@@ -273,28 +283,14 @@ class ParserOutput extends CacheTime {
 			'enableSectionEditLinks' => true,
 			'unwrap' => false,
 			'deduplicateStyles' => true,
+			'wrapperDivClass' => $this->getWrapperDivClass(),
 		];
 		$text = $this->mText;
 
 		Hooks::runWithoutAbort( 'ParserOutputPostCacheTransform', [ $this, &$text, &$options ] );
 
-		if ( $options['unwrap'] !== false ) {
-			$start = Html::openElement( 'div', [
-				'class' => 'mw-parser-output'
-			] );
-			$startLen = strlen( $start );
-			$end = Html::closeElement( 'div' );
-			$endPos = strrpos( $text, $end );
-			$endLen = strlen( $end );
-
-			if ( substr( $text, 0, $startLen ) === $start && $endPos !== false
-				// if the closing div is followed by real content, bail out of unwrapping
-				&& preg_match( '/^(?>\s*<!--.*?-->)*\s*$/s', substr( $text, $endPos + $endLen ) )
-			) {
-				$text = substr( $text, $startLen );
-				$text = substr( $text, 0, $endPos - $startLen )
-					. substr( $text, $endPos - $startLen + $endLen );
-			}
+		if ( $options['wrapperDivClass'] !== '' && !$options['unwrap'] ) {
+			$text = Html::rawElement( 'div', [ 'class' => $options['wrapperDivClass'] ], $text );
 		}
 
 		if ( $options['enableSectionEditLinks'] ) {
@@ -365,6 +361,34 @@ class ParserOutput extends CacheTime {
 		return $text;
 	}
 
+	/**
+	 * Add a CSS class to use for the wrapping div. If no class is given, no wrapper is added.
+	 *
+	 * @param string $class
+	 */
+	public function addWrapperDivClass( $class ) {
+		$this->mWrapperDivClasses[$class] = true;
+	}
+
+	/**
+	 * Clears the CSS class to use for the wrapping div, effectively disabling the wrapper div
+	 * until addWrapperDivClass() is called.
+	 */
+	public function clearWrapperDivClass() {
+		$this->mWrapperDivClasses = [];
+	}
+
+	/**
+	 * Returns the class (or classes) to be used with the wrapper div for this otuput.
+	 * If there is no wrapper class given, no wrapper div should be added.
+	 * The wrapper div is added automatically by getText().
+	 *
+	 * @return string
+	 */
+	public function getWrapperDivClass() {
+		return implode( ' ', array_keys( $this->mWrapperDivClasses ) );
+	}
+
 	/**
 	 * @param int $id
 	 * @since 1.28
diff --git a/tests/phpunit/includes/api/ApiParseTest.php b/tests/phpunit/includes/api/ApiParseTest.php
index 8a40266e44..7f2c1a6d5f 100644
--- a/tests/phpunit/includes/api/ApiParseTest.php
+++ b/tests/phpunit/includes/api/ApiParseTest.php
@@ -77,11 +77,15 @@ class ApiParseTest extends ApiTestCase {
 			$expectedEnd = "</div>";
 			$this->assertSame( $expectedEnd, substr( $html, -strlen( $expectedEnd ) ) );
 
+			$unexpectedEnd = '#<!-- \nNewPP limit report|' .
+				'<!--\nTransclusion expansion time report#s';
+			$this->assertNotRegExp( $unexpectedEnd, $html );
+
 			$html = substr( $html, 0, strlen( $html ) - strlen( $expectedEnd ) );
 		} else {
 			$expectedEnd = '#\n<!-- \nNewPP limit report\n(?>.+?\n-->)\n' .
 				'<!--\nTransclusion expansion time report \(%,ms,calls,template\)\n(?>.*?\n-->)\n' .
-				'</div>(\n<!-- Saved in parser cache (?>.*?\n -->)\n)?$#s';
+				'(\n<!-- Saved in parser cache (?>.*?\n -->)\n)?</div>$#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 d2ed4415bf..497c6b5bca 100644
--- a/tests/phpunit/includes/parser/ParserMethodsTest.php
+++ b/tests/phpunit/includes/parser/ParserMethodsTest.php
@@ -180,6 +180,18 @@ class ParserMethodsTest extends MediaWikiLangTestCase {
 		];
 	}
 
+	public function testWrapOutput() {
+		global $wgParser;
+		$title = Title::newFromText( 'foo' );
+		$po = new ParserOptions();
+		$wgParser->parse( 'Hello World', $title, $po );
+		$text = $wgParser->getOutput()->getText();
+
+		$this->assertContains( 'Hello World', $text );
+		$this->assertContains( '<div', $text );
+		$this->assertContains( 'class="mw-parser-output"', $text );
+	}
+
 	// @todo Add tests for cleanSig() / cleanSigInSig(), getSection(),
 	// replaceSection(), getPreloadText()
 }
diff --git a/tests/phpunit/includes/parser/ParserOutputTest.php b/tests/phpunit/includes/parser/ParserOutputTest.php
index b08ba6c484..439b24d37e 100644
--- a/tests/phpunit/includes/parser/ParserOutputTest.php
+++ b/tests/phpunit/includes/parser/ParserOutputTest.php
@@ -89,6 +89,59 @@ class ParserOutputTest extends MediaWikiTestCase {
 		$this->assertArrayNotHasKey( 'foo', $properties );
 	}
 
+	/**
+	 * @covers ParserOutput::getWrapperDivClass
+	 * @covers ParserOutput::addWrapperDivClass
+	 * @covers ParserOutput::clearWrapperDivClass
+	 * @covers ParserOutput::getText
+	 */
+	public function testWrapperDivClass() {
+		$po = new ParserOutput();
+
+		$po->setText( 'Kittens' );
+		$this->assertContains( 'Kittens', $po->getText() );
+		$this->assertNotContains( '<div', $po->getText() );
+		$this->assertSame( 'Kittens', $po->getRawText() );
+
+		$po->addWrapperDivClass( 'foo' );
+		$text = $po->getText();
+		$this->assertContains( 'Kittens', $text );
+		$this->assertContains( '<div', $text );
+		$this->assertContains( 'class="foo"', $text );
+
+		$po->addWrapperDivClass( 'bar' );
+		$text = $po->getText();
+		$this->assertContains( 'Kittens', $text );
+		$this->assertContains( '<div', $text );
+		$this->assertContains( 'class="foo bar"', $text );
+
+		$po->addWrapperDivClass( 'bar' ); // second time does nothing, no "foo bar bar".
+		$text = $po->getText( [ 'unwrap' => true ] );
+		$this->assertContains( 'Kittens', $text );
+		$this->assertNotContains( '<div', $text );
+		$this->assertNotContains( 'class="foo bar"', $text );
+
+		$text = $po->getText( [ 'wrapperDivClass' => '' ] );
+		$this->assertContains( 'Kittens', $text );
+		$this->assertNotContains( '<div', $text );
+		$this->assertNotContains( 'class="foo bar"', $text );
+
+		$text = $po->getText( [ 'wrapperDivClass' => 'xyzzy' ] );
+		$this->assertContains( 'Kittens', $text );
+		$this->assertContains( '<div', $text );
+		$this->assertContains( 'class="xyzzy"', $text );
+		$this->assertNotContains( 'class="foo bar"', $text );
+
+		$text = $po->getRawText();
+		$this->assertSame( 'Kittens', $text );
+
+		$po->clearWrapperDivClass();
+		$text = $po->getText();
+		$this->assertContains( 'Kittens', $text );
+		$this->assertNotContains( '<div', $text );
+		$this->assertNotContains( 'class="foo bar"', $text );
+	}
+
 	/**
 	 * @covers ParserOutput::getText
 	 * @dataProvider provideGetText
@@ -111,7 +164,7 @@ class ParserOutputTest extends MediaWikiTestCase {
 	public static function provideGetText() {
 		// phpcs:disable Generic.Files.LineLength
 		$text = <<<EOF
-<div class="mw-parser-output"><p>Test document.
+<p>Test document.
 </p>
 <mw:toc><div id="toc" class="toc"><div class="toctitle"><h2>Contents</h2></div>
 <ul>
@@ -136,7 +189,7 @@ class ParserOutputTest extends MediaWikiTestCase {
 </p>
 <h2><span class="mw-headline" id="Section_3">Section 3</span><mw:editsection page="Test Page" section="4">Section 3</mw:editsection></h2>
 <p>Three
-</p></div>
+</p>
 EOF;
 
 		$dedupText = <<<EOF
@@ -155,7 +208,7 @@ EOF;
 		return [
 			'No options' => [
 				[], $text, <<<EOF
-<div class="mw-parser-output"><p>Test document.
+<p>Test document.
 </p>
 <div id="toc" class="toc"><div class="toctitle"><h2>Contents</h2></div>
 <ul>
@@ -180,12 +233,12 @@ EOF;
 </p>
 <h2><span class="mw-headline" id="Section_3">Section 3</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/w/index.php?title=Test_Page&amp;action=edit&amp;section=4" title="Edit section: Section 3">edit</a><span class="mw-editsection-bracket">]</span></span></h2>
 <p>Three
-</p></div>
+</p>
 EOF
 			],
 			'Disable section edit links' => [
 				[ 'enableSectionEditLinks' => false ], $text, <<<EOF
-<div class="mw-parser-output"><p>Test document.
+<p>Test document.
 </p>
 <div id="toc" class="toc"><div class="toctitle"><h2>Contents</h2></div>
 <ul>
@@ -210,11 +263,11 @@ EOF
 </p>
 <h2><span class="mw-headline" id="Section_3">Section 3</span></h2>
 <p>Three
-</p></div>
+</p>
 EOF
 			],
-			'Disable TOC' => [
-				[ 'allowTOC' => false ], $text, <<<EOF
+			'Disable TOC, but wrap' => [
+				[ 'allowTOC' => false, 'wrapperDivClass' => 'mw-parser-output' ], $text, <<<EOF
 <div class="mw-parser-output"><p>Test document.
 </p>
 
@@ -231,44 +284,6 @@ EOF
 <p>Three
 </p></div>
 EOF
-			],
-			'Unwrap text' => [
-				[ 'unwrap' => true ], $text, <<<EOF
-<p>Test document.
-</p>
-<div id="toc" class="toc"><div class="toctitle"><h2>Contents</h2></div>
-<ul>
-<li class="toclevel-1 tocsection-1"><a href="#Section_1"><span class="tocnumber">1</span> <span class="toctext">Section 1</span></a></li>
-<li class="toclevel-1 tocsection-2"><a href="#Section_2"><span class="tocnumber">2</span> <span class="toctext">Section 2</span></a>
-<ul>
-<li class="toclevel-2 tocsection-3"><a href="#Section_2.1"><span class="tocnumber">2.1</span> <span class="toctext">Section 2.1</span></a></li>
-</ul>
-</li>
-<li class="toclevel-1 tocsection-4"><a href="#Section_3"><span class="tocnumber">3</span> <span class="toctext">Section 3</span></a></li>
-</ul>
-</div>
-
-<h2><span class="mw-headline" id="Section_1">Section 1</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/w/index.php?title=Test_Page&amp;action=edit&amp;section=1" title="Edit section: Section 1">edit</a><span class="mw-editsection-bracket">]</span></span></h2>
-<p>One
-</p>
-<h2><span class="mw-headline" id="Section_2">Section 2</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/w/index.php?title=Test_Page&amp;action=edit&amp;section=2" title="Edit section: Section 2">edit</a><span class="mw-editsection-bracket">]</span></span></h2>
-<p>Two
-</p>
-<h3><span class="mw-headline" id="Section_2.1">Section 2.1</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/w/index.php?title=Test_Page&amp;action=edit&amp;section=3" title="Edit section: Section 2.1">edit</a><span class="mw-editsection-bracket">]</span></span></h3>
-<p>Two point one
-</p>
-<h2><span class="mw-headline" id="Section_3">Section 3</span><span class="mw-editsection"><span class="mw-editsection-bracket">[</span><a href="/w/index.php?title=Test_Page&amp;action=edit&amp;section=4" title="Edit section: Section 3">edit</a><span class="mw-editsection-bracket">]</span></span></h2>
-<p>Three
-</p>
-EOF
-			],
-			'Unwrap without a mw-parser-output wrapper' => [
-				[ 'unwrap' => true ], '<div class="foobar">Content</div>', '<div class="foobar">Content</div>'
-			],
-			'Unwrap with extra comment at end' => [
-				[ 'unwrap' => true ], '<div class="mw-parser-output"><p>Test document.</p></div>
-<!-- Saved in parser cache... -->', '<p>Test document.</p>
-<!-- Saved in parser cache... -->'
 			],
 			'Style deduplication' => [
 				[], $dedupText, <<<EOF
-- 
2.20.1