Several code cleanups after getting rid of cloning

* Use the Html class to safely create HTML code.
* $this->referenceStack can not be null any more.
* $this->inReferencesGroup is not needed during output, only when
  parsing tags.
* Replace ReferencesStack::getGroupRefs() as well as deleteGroup()
  with a combined popGroup() that does both things.
* Extract the code responsible for the "responsive" behavior to a
  separate function.
* Some TestingAccessWrapper are not needed.

Change-Id: Ie1cf2533d7417ae2f6647664ff1145e37b814a39

src/Cite.php

@@ -24,6 +24,7 @@
 
 namespace Cite;
 
+use Html;
 use Parser;
 use Sanitizer;
 use StatusValue;
@@ -104,9 +105,15 @@ class Cite {
 		$this->referenceStack = new ReferenceStack( $this->errorReporter );
 		$anchorFormatter = new AnchorFormatter( $messageLocalizer );
 		$this->footnoteMarkFormatter = new FootnoteMarkFormatter(
-			$this->errorReporter, $anchorFormatter, $messageLocalizer );
+			$this->errorReporter,
+			$anchorFormatter,
+			$messageLocalizer
+		);
 		$this->referencesFormatter = new ReferencesFormatter(
-			$this->errorReporter, $anchorFormatter, $messageLocalizer );
+			$this->errorReporter,
+			$anchorFormatter,
+			$messageLocalizer
+		);
 	}
 
 	/**
@@ -314,7 +321,10 @@ class Cite {
 			if ( !$status->isOK() ) {
 				foreach ( $status->getErrors() as $error ) {
 					$this->mReferencesErrors[] = $this->errorReporter->halfParsed(
-						$parser, $error['message'], ...$error['params'] );
+						$parser,
+						$error['message'],
+						...$error['params']
+					);
 				}
 			} else {
 				$groupRefs = $this->referenceStack->getGroupRefs( $group );
@@ -326,11 +336,15 @@ class Cite {
 						// adds error message to the original ref
 						// TODO: report these errors the same way as the others, rather than a
 						//  special case to append to the second one's content.
-						$text =
-							$groupRefs[$name]['text'] . ' ' .
-							$this->errorReporter->plain(
-								$parser, 'cite_error_references_duplicate_key', $name );
-						$this->referenceStack->setRefText( $group, $name, $text );
+						$this->referenceStack->setRefText(
+							$group,
+							$name,
+							$groupRefs[$name]['text'] . ' ' . $this->errorReporter->plain(
+								$parser,
+								'cite_error_references_duplicate_key',
+								$name
+							)
+						);
 					}
 				}
 			}
@@ -417,12 +431,7 @@ class Cite {
 		array $argv,
 		Parser $parser
 	) : string {
-		global $wgCiteResponsiveReferences;
-
-		$status = $this->parseArguments(
-			$argv,
-			[ 'group', 'responsive' ]
-		);
+		$status = $this->parseArguments( $argv, [ 'group', 'responsive' ] );
 		[ 'group' => $group, 'responsive' => $responsive ] = $status->getValue();
 		$this->inReferencesGroup = $group ?? self::DEFAULT_GROUP;
 
@@ -451,13 +460,6 @@ class Cite {
 			$parser->recursiveTagParse( $text );
 		}
 
-		// FIXME: This feature is not covered by parser tests!
-		if ( isset( $argv['responsive'] ) ) {
-			$responsive = $responsive !== '0';
-		} else {
-			$responsive = $wgCiteResponsiveReferences;
-		}
-
 		if ( !$status->isOK() ) {
 			// Bail out with an error.
 			$error = $status->getErrors()[0];
@@ -482,21 +484,24 @@ class Cite {
 	 *
 	 * @param Parser $parser
 	 * @param string $group
-	 * @param bool $responsive
+	 * @param string|null $responsive Defaults to $wgCiteResponsiveReferences when not set
 	 *
 	 * @return string HTML
 	 */
-	private function formatReferences( Parser $parser, string $group, bool $responsive ) : string {
-		$ret = $this->referencesFormatter->formatReferences(
+	private function formatReferences(
+		Parser $parser,
+		string $group,
+		string $responsive = null
+	) : string {
+		global $wgCiteResponsiveReferences;
+
+		return $this->referencesFormatter->formatReferences(
 			$parser,
-			$this->referenceStack->getGroupRefs( $group ),
-			$responsive,
-			$this->isSectionPreview );
-
-		// done, clean up so we can reuse the group
-		$this->referenceStack->deleteGroup( $group );
-
-		return $ret;
+			$this->referenceStack->popGroup( $group ),
+			// FIXME: The responsive feature is not covered by parser tests!
+			(bool)( $responsive ?? $wgCiteResponsiveReferences ),
+			$this->isSectionPreview
+		);
 	}
 
 	/**
@@ -513,18 +518,10 @@ class Cite {
 	 * @return string HTML
 	 */
 	public function checkRefsNoReferences( Parser $parser, bool $isSectionPreview ) : string {
-		global $wgCiteResponsiveReferences;
-
-		if ( !$this->referenceStack ) {
-			return '';
-		}
-
 		$s = '';
 		foreach ( $this->referenceStack->getGroups() as $group ) {
 			if ( $group === self::DEFAULT_GROUP || $isSectionPreview ) {
-				$this->inReferencesGroup = $group;
-				$s .= $this->formatReferences( $parser, $group, $wgCiteResponsiveReferences );
-				$this->inReferencesGroup = null;
+				$s .= $this->formatReferences( $parser, $group );
 			} else {
 				$s .= "\n<br />" . $this->errorReporter->halfParsed(
 					$parser,
@@ -536,20 +533,24 @@ class Cite {
 		if ( $isSectionPreview && $s !== '' ) {
 			$headerMsg = wfMessage( 'cite_section_preview_references' );
 			if ( !$headerMsg->isDisabled() ) {
-				$s = '<h2 id="mw-ext-cite-cite_section_preview_references_header" >'
-					. $headerMsg->escaped()
-					. '</h2>' . $s;
+				$s = Html::element(
+					'h2',
+					[ 'id' => 'mw-ext-cite-cite_section_preview_references_header' ],
+					$headerMsg->text()
+				) . $s;
 			}
 			// provide a preview of references in its own section
-			$s = "\n" . '<div class="mw-ext-cite-cite_section_preview_references" >' . $s . '</div>';
+			$s = "\n" . Html::rawElement(
+				'div',
+				[ 'class' => 'mw-ext-cite-cite_section_preview_references' ],
+				$s
+			);
 		}
 		return $s;
 	}
 
 	public function __clone() {
-		if ( $this->referenceStack ) {
-			$this->referenceStack = clone $this->referenceStack;
-		}
+		$this->referenceStack = clone $this->referenceStack;
 	}
 
 }

src/FootnoteMarkFormatter.php

@@ -105,9 +105,12 @@ class FootnoteMarkFormatter {
 			return null;
 		}
 
-		return $this->linkLabels[$group][$number - 1]
-			?? $this->errorReporter->plain(
-				$parser, 'cite_error_no_link_label_group', $group, $message );
+		return $this->linkLabels[$group][$number - 1] ?? $this->errorReporter->plain(
+			$parser,
+			'cite_error_no_link_label_group',
+			$group,
+			$message
+		);
 	}
 
 }

src/ReferenceStack.php

@@ -332,7 +332,7 @@ class ReferenceStack {
 				unset( $this->refs[$group][$lookup] );
 				if ( $this->refs[$group] === [] ) {
 					// TODO: Unsetting is unecessary.
-					$this->deleteGroup( $group );
+					$this->popGroup( $group );
 				}
 				// TODO: else, don't we need to decrement groupRefSequence?
 				break;
@@ -354,11 +354,15 @@ class ReferenceStack {
 	 * Clear state for a single group.
 	 *
 	 * @param string $group
+	 *
+	 * @return array[] The references from the removed group
 	 */
-	public function deleteGroup( string $group ) {
+	public function popGroup( string $group ) {
+		$refs = $this->getGroupRefs( $group );
 		unset( $this->refs[$group] );
 		unset( $this->groupRefSequence[$group] );
 		unset( $this->extendsCount[$group] );
+		return $refs;
 	}
 
 	/**

src/ReferencesFormatter.php

@@ -67,11 +67,36 @@ class ReferencesFormatter {
 			return '';
 		}
 
-		// Add new lines between the list items (ref entries) to avoid confusing tidy (T15073).
-		// Note: This builds a string of wikitext, not html.
-		$parserInput = "\n";
-		/** @var string|bool $indented */
-		$indented = false;
+		$wikitext = $this->formatRefsList( $parser, $groupRefs, $isSectionPreview );
+
+		// Live hack: parse() adds two newlines on WM, can't reproduce it locally -ævar
+		$html = rtrim( $parser->recursiveTagParse( $wikitext ), "\n" );
+
+		if ( $responsive ) {
+			$wrapClasses = [ 'mw-references-wrap' ];
+			if ( count( $groupRefs ) > 10 ) {
+				$wrapClasses[] = 'mw-references-columns';
+			}
+			// Use a DIV wrap because column-count on a list directly is broken in Chrome.
+			// See https://bugs.chromium.org/p/chromium/issues/detail?id=498730.
+			return Html::rawElement( 'div', [ 'class' => $wrapClasses ], $html );
+		}
+
+		return $html;
+	}
+
+	/**
+	 * @param Parser $parser
+	 * @param array[] $groupRefs
+	 * @param bool $isSectionPreview
+	 *
+	 * @return string Wikitext
+	 */
+	private function formatRefsList(
+		Parser $parser,
+		array $groupRefs,
+		bool $isSectionPreview
+	) : string {
 		// After sorting the list, we can assume that references are in the same order as their
 		// numbering.  Subreferences will come immediately after their parent.
 		uasort(
@@ -81,6 +106,12 @@ class ReferencesFormatter {
 				return $cmp ?: ( $a['extendsIndex'] ?? 0 ) - ( $b['extendsIndex'] ?? 0 );
 			}
 		);
+
+		// Add new lines between the list items (ref entries) to avoid confusing tidy (T15073).
+		// Note: This builds a string of wikitext, not html.
+		$parserInput = "\n";
+		/** @var string|bool $indented */
+		$indented = false;
 		foreach ( $groupRefs as $key => $value ) {
 			// Make sure the parent is not a subreference.
 			// FIXME: Move to a validation function.
@@ -106,21 +137,7 @@ class ReferencesFormatter {
 			$parserInput .= $this->formatListItem( $parser, $key, $value, $isSectionPreview ) . "\n";
 		}
 		$parserInput .= $this->closeIndention( $indented );
-		$parserInput = Html::rawElement( 'ol', [ 'class' => [ 'references' ] ], $parserInput );
-		// Live hack: parse() adds two newlines on WM, can't reproduce it locally -ævar
-		$ret = rtrim( $parser->recursiveTagParse( $parserInput ), "\n" );
-
-		if ( $responsive ) {
-			// Use a DIV wrap because column-count on a list directly is broken in Chrome.
-			// See https://bugs.chromium.org/p/chromium/issues/detail?id=498730.
-			$wrapClasses = [ 'mw-references-wrap' ];
-			if ( count( $groupRefs ) > 10 ) {
-				$wrapClasses[] = 'mw-references-columns';
-			}
-			$ret = Html::rawElement( 'div', [ 'class' => $wrapClasses ], $ret );
-		}
-
-		return $ret;
+		return Html::rawElement( 'ol', [ 'class' => [ 'references' ] ], $parserInput );
 	}
 
 	/**
@@ -137,8 +154,6 @@ class ReferencesFormatter {
 	}
 
 	/**
-	 * Format a single entry for the referencesFormat() function
-	 *
 	 * @param Parser $parser
 	 * @param string|int $key The key of the reference
 	 * @param array $val A single reference as documented at {@see ReferenceStack::$refs}
@@ -219,8 +234,6 @@ class ReferencesFormatter {
 	}
 
 	/**
-	 * Returns formatted reference text
-	 *
 	 * @param string|int $key
 	 * @param ?string $text
 	 * @param bool $isSectionPreview
@@ -297,6 +310,7 @@ class ReferencesFormatter {
 	 * use messages from the content language) I'm rolling my own.
 	 *
 	 * @param string[] $arr The array to format
+	 *
 	 * @return string
 	 */
 	private function listToText( array $arr ) : string {

tests/phpunit/CiteIntegrationTest.php

@@ -38,7 +38,7 @@ class CiteIntegrationTest extends \MediaWikiIntegrationTestCase {
 
 		$mockReferenceStack = $this->createMock( ReferenceStack::class );
 		$mockReferenceStack->method( 'getGroups' )->willReturn( array_keys( $initialRefs ) );
-		$mockReferenceStack->method( 'getGroupRefs' )->willReturnCallback( function ( $group ) use (
+		$mockReferenceStack->method( 'popGroup' )->willReturnCallback( function ( $group ) use (
 			$initialRefs
 		) {
 			return $initialRefs[$group];
@@ -89,8 +89,8 @@ class CiteIntegrationTest extends \MediaWikiIntegrationTestCase {
 					]
 				],
 				true,
-				"\n" . '<div class="mw-ext-cite-cite_section_preview_references" >' .
-				'<h2 id="mw-ext-cite-cite_section_preview_references_header" >' .
+				"\n" . '<div class="mw-ext-cite-cite_section_preview_references">' .
+				'<h2 id="mw-ext-cite-cite_section_preview_references_header">' .
 				'(cite_section_preview_references)</h2><references /></div>'
 			],
 			'Named group' => [
@@ -113,8 +113,8 @@ class CiteIntegrationTest extends \MediaWikiIntegrationTestCase {
 					]
 				],
 				true,
-				"\n" . '<div class="mw-ext-cite-cite_section_preview_references" >' .
-				'<h2 id="mw-ext-cite-cite_section_preview_references_header" >' .
+				"\n" . '<div class="mw-ext-cite-cite_section_preview_references">' .
+				'<h2 id="mw-ext-cite-cite_section_preview_references_header">' .
 				'(cite_section_preview_references)</h2><references /></div>'
 			]
 		];

tests/phpunit/ReferenceMessageLocalizerTest.php

@@ -9,6 +9,7 @@ use MediaWikiIntegrationTestCase;
  * @coversDefaultClass \Cite\ReferenceMessageLocalizer
  */
 class ReferenceMessageLocalizerTest extends MediaWikiIntegrationTestCase {
+
 	/**
 	 * @covers ::msg
 	 */
@@ -18,4 +19,5 @@ class ReferenceMessageLocalizerTest extends MediaWikiIntegrationTestCase {
 			'(cite_reference_link_prefix)',
 			$localizer->msg( 'cite_reference_link_prefix' )->plain() );
 	}
+
 }

tests/phpunit/unit/CiteUnitTest.php

@@ -322,10 +322,12 @@ class CiteUnitTest extends MediaWikiUnitTestCase {
 		global $wgCiteResponsiveReferences;
 		$wgCiteResponsiveReferences = false;
 
+		/** @var Parser $parser */
+		$parser = $this->createMock( Parser::class );
+
 		$cite = $this->newCite();
 		/** @var Cite $spy */
 		$spy = TestingAccessWrapper::newFromObject( $cite );
-		/** @var ErrorReporter $mockErrorReporter */
 		$spy->errorReporter = $this->createMock( ErrorReporter::class );
 		$spy->errorReporter->method( 'halfParsed' )->willReturnCallback(
 			function ( $parser, ...$args ) {
@@ -334,11 +336,11 @@ class CiteUnitTest extends MediaWikiUnitTestCase {
 		);
 		$spy->referencesFormatter = $this->createMock( ReferencesFormatter::class );
 		$spy->referencesFormatter->method( 'formatReferences' )
-			->with( $this->anything(), $this->anything(), $expectedResponsive, false )
+			->with( $parser, [], $expectedResponsive, false )
 			->willReturn( 'references!' );
 		$spy->isSectionPreview = false;
 		$spy->referenceStack = $this->createMock( ReferenceStack::class );
-		$spy->referenceStack->method( 'getGroupRefs' )
+		$spy->referenceStack->method( 'popGroup' )
 			->with( $expectedInReferencesGroup )->willReturn( [] );
 		if ( $expectedRollbackCount === 0 ) {
 			$spy->referenceStack->expects( $this->never() )->method( 'rollbackRefs' );
@@ -347,8 +349,7 @@ class CiteUnitTest extends MediaWikiUnitTestCase {
 				->with( $expectedRollbackCount )->willReturn( [ [ [], 't' ] ] );
 		}
 
-		$output = $spy->guardedReferences(
-			$text, $argv, $this->createMock( Parser::class ) );
+		$output = $spy->guardedReferences( $text, $argv, $parser );
 		$this->assertSame( $expectedOutput, $output );
 	}
 

tests/phpunit/unit/FootnoteMarkFormatterTest.php

@@ -47,11 +47,11 @@ class FootnoteMarkFormatterTest extends MediaWikiUnitTestCase {
 		);
 		$mockParser = $this->createMock( Parser::class );
 		$mockParser->method( 'recursiveTagParse' )->willReturnArgument( 0 );
-		/** @var FootnoteMarkFormatter $formatter */
-		$formatter = TestingAccessWrapper::newFromObject( new FootnoteMarkFormatter(
+		$formatter = new FootnoteMarkFormatter(
 			$mockErrorReporter,
 			$anchorFormatter,
-			$mockMessageLocalizer ) );
+			$mockMessageLocalizer
+		);
 
 		$output = $formatter->linkRef( $mockParser, $group, $ref );
 		$this->assertSame( $expectedOutput, $output );
@@ -183,4 +183,5 @@ class FootnoteMarkFormatterTest extends MediaWikiUnitTestCase {
 		yield [ 'å', 1, 'foo', 'å β' ];
 		yield [ 'cite_error_no_link_label_group|foo|cite_link_label_group-foo', 4, 'foo', 'a b c' ];
 	}
+
 }

tests/phpunit/unit/ReferenceStackTest.php

@@ -1013,13 +1013,13 @@ class ReferenceStackTest extends MediaWikiUnitTestCase {
 	}
 
 	/**
-	 * @covers ::deleteGroup
+	 * @covers ::popGroup
 	 */
 	public function testRemovals() {
 		$stack = $this->newStack();
 		$stack->refs = [ 'group1' => [], 'group2' => [] ];
 
-		$stack->deleteGroup( 'group1' );
+		$this->assertSame( [], $stack->popGroup( 'group1' ) );
 		$this->assertSame( [ 'group2' => [] ], $stack->refs );
 	}
 

tests/phpunit/unit/ReferencesFormatterTest.php

@@ -25,13 +25,14 @@ class ReferencesFormatterTest extends MediaWikiUnitTestCase {
 	public function testFormatReferences( array $refs, string $expectedOutput ) {
 		$mockParser = $this->createMock( Parser::class );
 		$mockParser->method( 'recursiveTagParse' )->willReturnArgument( 0 );
-		/** @var Parser $mockParser */
+
 		$mockErrorReporter = $this->createMock( ErrorReporter::class );
 		$mockErrorReporter->method( 'plain' )->willReturnCallback(
 			function ( $parser, ...$args ) {
 				return '(' . implode( '|', $args ) . ')';
 			}
 		);
+
 		$mockMessageLocalizer = $this->createMock( ReferenceMessageLocalizer::class );
 		$mockMessageLocalizer->method( 'msg' )->willReturnCallback(
 			function ( ...$args ) {
@@ -41,14 +42,14 @@ class ReferencesFormatterTest extends MediaWikiUnitTestCase {
 			}
 		);
 
+		/** @var Parser $mockParser */
 		/** @var ErrorReporter $mockErrorReporter */
 		/** @var ReferenceMessageLocalizer $mockMessageLocalizer */
-		/** @var ReferencesFormatter $formatter */
-		$formatter = TestingAccessWrapper::newFromObject( new ReferencesFormatter(
+		$formatter = new ReferencesFormatter(
 			$mockErrorReporter,
 			$this->createMock( AnchorFormatter::class ),
 			$mockMessageLocalizer
-		) );
+		);
 
 		$output = $formatter->formatReferences( $mockParser, $refs, true, false );
 		$this->assertSame( $expectedOutput, $output );
@@ -169,6 +170,7 @@ class ReferencesFormatterTest extends MediaWikiUnitTestCase {
 	 * @dataProvider provideCloseIndention
 	 */
 	public function testCloseIndention( $closingLi, $expectedOutput ) {
+		/** @var ReferencesFormatter $formatter */
 		$formatter = TestingAccessWrapper::newFromObject( new ReferencesFormatter(
 			$this->createMock( ErrorReporter::class ),
 			$this->createMock( AnchorFormatter::class ),