Track warnings separately in ReferenceStack

Check out how this gets rid of so many "to do" as well as
"deprecated" comments.

Next qustion: The elements in the stack become more and more
complicated. It's probably worth converting them from arrays into
first-class objects. But this is for another patch.

Bug: T353266
Change-Id: If14acd1070617ca8c4d15be6b1759bd47ead4926

src/Cite.php

@@ -77,7 +77,7 @@ class Cite {
 		$this->isSectionPreview = $parser->getOptions()->getIsSectionPreview();
 		$messageLocalizer = new ReferenceMessageLocalizer( $parser->getContentLanguage() );
 		$this->errorReporter = new ErrorReporter( $messageLocalizer );
-		$this->referenceStack = new ReferenceStack( $this->errorReporter );
+		$this->referenceStack = new ReferenceStack();
 		$anchorFormatter = new AnchorFormatter( $messageLocalizer );
 		$this->footnoteMarkFormatter = new FootnoteMarkFormatter(
 			$this->errorReporter,
@@ -172,16 +172,10 @@ class Cite {
 			} elseif ( $groupRefs[$name]['text'] !== $text ) {
 				// two refs with same key and different content
 				// 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.
-				$this->referenceStack->appendText(
+				$this->referenceStack->warning(
 					$group,
 					$name,
-					' ' . $this->errorReporter->plain(
-						$parser,
-						'cite_error_references_duplicate_key',
-						$name
-					)
+					'cite_error_references_duplicate_key', $name
 				);
 			}
 			return '';
@@ -198,7 +192,7 @@ class Cite {
 
 		// @phan-suppress-next-line PhanParamTooFewUnpack No good way to document it.
 		$ref = $this->referenceStack->pushRef(
-			$parser, $parser->getStripState(), $text, $argv, ...array_values( $arguments ) );
+			$parser->getStripState(), $text, $argv, ...array_values( $arguments ) );
 		return $ref
 			? $this->footnoteMarkFormatter->linkRef( $parser, $group, $ref )
 			: '';

src/ReferenceStack.php

@@ -3,7 +3,6 @@
 namespace Cite;
 
 use LogicException;
-use Parser;
 use StripState;
 
 /**
@@ -40,6 +39,7 @@ class ReferenceStack {
 	 * - 'text': The content inside the <ref>…</ref> tag. Null for <ref /> without content. Also
 	 *       null for <ref></ref> without any non-whitespace content.
 	 * - 'dir': Direction of the text. Should either be "ltr" or "rtl".
+	 * - 'warnings': Error messages attached to this reference.
 	 *
 	 * @var array[][]
 	 */
@@ -65,21 +65,12 @@ class ReferenceStack {
 	 */
 	private array $refCallStack = [];
 
-	/**
-	 * @deprecated We should be able to push this responsibility to calling code.
-	 */
-	private ErrorReporter $errorReporter;
-
 	private const ACTION_ASSIGN = 'assign';
 	private const ACTION_INCREMENT = 'increment';
 	private const ACTION_NEW_FROM_PLACEHOLDER = 'new-from-placeholder';
 	private const ACTION_NEW = 'new';
 	private const PARENT_REF_PLACEHOLDER = '__placeholder__';
 
-	public function __construct( ErrorReporter $errorReporter ) {
-		$this->errorReporter = $errorReporter;
-	}
-
 	/**
 	 * Leave a mark in the stack which matches an invalid ref tag.
 	 */
@@ -90,7 +81,6 @@ class ReferenceStack {
 	/**
 	 * Populate $this->refs and $this->refCallStack based on input and arguments to <ref>
 	 *
-	 * @param Parser $parser
 	 * @param StripState $stripState
 	 * @param ?string $text Content from the <ref> tag
 	 * @param string[] $argv
@@ -104,7 +94,6 @@ class ReferenceStack {
 	 * @suppress PhanTypePossiblyInvalidDimOffset To many complaints about array indizes
 	 */
 	public function pushRef(
-		Parser $parser,
 		StripState $stripState,
 		?string $text,
 		array $argv,
@@ -176,11 +165,7 @@ class ReferenceStack {
 					!== $stripState->unstripBoth( $ref['text'] )
 				) {
 					// two refs with same name and different text
-					// add error message to the original ref
-					// TODO: standardize error display and move to `validateRef`.
-					$ref['text'] .= ' ' . $this->errorReporter->plain(
-						$parser, 'cite_error_references_duplicate_key', $name
-					);
+					$ref['warnings'][] = [ 'cite_error_references_duplicate_key', $name ];
 				}
 				$action = self::ACTION_INCREMENT;
 			}
@@ -210,13 +195,7 @@ class ReferenceStack {
 			}
 		} elseif ( $extends && $ref['extends'] !== $extends ) {
 			// TODO: Change the error message to talk about "conflicting content or parent"?
-			$error = $this->errorReporter->plain( $parser, 'cite_error_references_duplicate_key',
-				$name );
-			if ( isset( $ref['text'] ) ) {
-				$ref['text'] .= ' ' . $error;
-			} else {
-				$ref['text'] = $error;
-			}
+			$ref['warnings'][] = [ 'cite_error_references_duplicate_key', $name ];
 		}
 
 		$this->refCallStack[] = [ $action, $ref['key'], $group, $name, $extends, $text, $argv ];
@@ -395,4 +374,11 @@ class ReferenceStack {
 		$this->refs[$group][$name]['text'] .= $text;
 	}
 
+	/**
+	 * @deprecated Temporary helper function
+	 */
+	public function warning( string $group, string $name, string $message, ...$parameters ): void {
+		$this->refs[$group][$name]['warnings'][] = [ $message, ...$parameters ];
+	}
+
 }

src/ReferencesFormatter.php

@@ -95,15 +95,14 @@ class ReferencesFormatter {
 		$parserInput = "\n";
 		/** @var string|bool $indented */
 		$indented = false;
-		foreach ( $groupRefs as $key => $value ) {
+		foreach ( $groupRefs as $key => &$value ) {
 			// Make sure the parent is not a subreference.
 			// FIXME: Move to a validation function.
 			if ( isset( $value['extends'] ) &&
 				isset( $groupRefs[$value['extends']]['extends'] )
 			) {
-				$value['text'] = ( $value['text'] ?? '' ) . ' ' .
-					$this->errorReporter->plain( $parser, 'cite_error_ref_nested_extends',
-						$value['extends'], $groupRefs[$value['extends']]['extends'] );
+				$value['warnings'][] = [ 'cite_error_ref_nested_extends',
+					$value['extends'], $groupRefs[$value['extends']]['extends'] ];
 			}
 
 			if ( !$indented && isset( $value['extends'] ) ) {
@@ -147,7 +146,7 @@ class ReferencesFormatter {
 	private function formatListItem(
 		Parser $parser, $key, array $val, bool $isSectionPreview
 	): string {
-		$text = $this->referenceText( $parser, $key, $val['text'] ?? null, $isSectionPreview );
+		$text = $this->referenceText( $parser, $key, $val, $isSectionPreview );
 		$error = '';
 		$extraAttributes = '';
 
@@ -209,14 +208,15 @@ class ReferencesFormatter {
 	/**
 	 * @param Parser $parser
 	 * @param string|int $key
-	 * @param ?string $text
+	 * @param array $ref
 	 * @param bool $isSectionPreview
 	 *
 	 * @return string
 	 */
 	private function referenceText(
-		Parser $parser, $key, ?string $text, bool $isSectionPreview
+		Parser $parser, $key, array $ref, bool $isSectionPreview
 	): string {
+		$text = $ref['text'] ?? null;
 		if ( $text === null ) {
 			return $this->errorReporter->plain( $parser,
 				$isSectionPreview
@@ -224,6 +224,13 @@ class ReferencesFormatter {
 					: 'cite_error_references_no_text', $key );
 		}
 
+		foreach ( $ref['warnings'] ?? [] as $warning ) {
+			// @phan-suppress-next-line PhanParamTooFewUnpack
+			$text .= ' ' . $this->errorReporter->plain( $parser, ...$warning );
+			// FIXME: We could use a StatusValue object to get rid of duplicates
+			break;
+		}
+
 		return '<span class="reference-text">' . rtrim( $text, "\n" ) . "</span>\n";
 	}
 

tests/phpunit/CiteIntegrationTest.php

@@ -35,7 +35,7 @@ class CiteIntegrationTest extends \MediaWikiIntegrationTestCase {
 			static fn ( $parser, ...$args ) => '(' . implode( '|', $args ) . ')'
 		);
 
-		$referenceStack = new ReferenceStack( $mockErrorReporter );
+		$referenceStack = new ReferenceStack();
 		TestingAccessWrapper::newFromObject( $referenceStack )->refs = $initialRefs;
 
 		$referencesFormatter = $this->createMock( ReferencesFormatter::class );

tests/phpunit/integration/CiteTest.php

@@ -199,7 +199,7 @@ class CiteTest extends \MediaWikiIntegrationTestCase {
 			static fn ( $parser, ...$args ) => '(' . implode( '|', $args ) . ')'
 		);
 
-		$referenceStack = new ReferenceStack( $errorReporter );
+		$referenceStack = new ReferenceStack();
 		/** @var ReferenceStack $stackSpy */
 		$stackSpy = TestingAccessWrapper::newFromObject( $referenceStack );
 		$stackSpy->refs = $initialRefs;
@@ -345,7 +345,10 @@ class CiteTest extends \MediaWikiIntegrationTestCase {
 				[],
 				[
 					'' => [
-						'a' => [ 'text' => 'text-1 (cite_error_references_duplicate_key|a)' ],
+						'a' => [
+							'text' => 'text-1',
+							'warnings' => [ [ 'cite_error_references_duplicate_key', 'a' ] ],
+						],
 					],
 				]
 			],

tests/phpunit/integration/ValidatorTest.php

@@ -2,7 +2,6 @@
 
 namespace Cite\Tests\Integration;
 
-use Cite\ErrorReporter;
 use Cite\ReferenceStack;
 use Cite\Validator;
 use Wikimedia\TestingAccessWrapper;
@@ -28,8 +27,7 @@ class ValidatorTest extends \MediaWikiIntegrationTestCase {
 		?string $dir,
 		?string $expected
 	) {
-		$errorReporter = $this->createMock( ErrorReporter::class );
-		$stack = new ReferenceStack( $errorReporter );
+		$stack = new ReferenceStack();
 		TestingAccessWrapper::newFromObject( $stack )->refs = $referencesStack;
 
 		$validator = new Validator( $stack, $inReferencesGroup, $isSectionPreview, true );

tests/phpunit/unit/ReferenceStackTest.php

@@ -2,10 +2,8 @@
 
 namespace Cite\Tests\Unit;
 
-use Cite\ErrorReporter;
 use Cite\ReferenceStack;
 use LogicException;
-use Parser;
 use StripState;
 use Wikimedia\TestingAccessWrapper;
 
@@ -38,7 +36,6 @@ class ReferenceStackTest extends \MediaWikiUnitTestCase {
 
 		for ( $i = 0; $i < count( $refs ); $i++ ) {
 			$result = $stack->pushRef(
-				$this->createNoOpMock( Parser::class ),
 				$mockStripState,
 				...$refs[$i]
 			);
@@ -387,8 +384,9 @@ class ReferenceStackTest extends \MediaWikiUnitTestCase {
 						'dir' => 'rtl',
 						'key' => 1,
 						'name' => 'a',
-						'text' => 'text-1 cite_error_references_duplicate_key',
+						'text' => 'text-1',
 						'number' => 1,
+						'warnings' => [ [ 'cite_error_references_duplicate_key', 'a' ] ],
 					]
 				],
 				[
@@ -398,8 +396,9 @@ class ReferenceStackTest extends \MediaWikiUnitTestCase {
 							'dir' => 'rtl',
 							'key' => 1,
 							'name' => 'a',
-							'text' => 'text-1 cite_error_references_duplicate_key',
+							'text' => 'text-1',
 							'number' => 1,
+							'warnings' => [ [ 'cite_error_references_duplicate_key', 'a' ] ],
 						]
 					]
 				],
@@ -980,7 +979,6 @@ class ReferenceStackTest extends \MediaWikiUnitTestCase {
 		$mockStripState = $this->createMock( StripState::class );
 		$mockStripState->method( 'unstripBoth' )->willReturnArgument( 0 );
 		$stack->pushRef(
-			$this->createNoOpMock( Parser::class ),
 			$mockStripState,
 			'text', [],
 			'foo', null, 'a', null, 'rtl'
@@ -1039,9 +1037,7 @@ class ReferenceStackTest extends \MediaWikiUnitTestCase {
 	 * @return ReferenceStack
 	 */
 	private function newStack() {
-		$errorReporter = $this->createMock( ErrorReporter::class );
-		$errorReporter->method( 'plain' )->willReturnArgument( 1 );
-		return TestingAccessWrapper::newFromObject( new ReferenceStack( $errorReporter ) );
+		return TestingAccessWrapper::newFromObject( new ReferenceStack() );
 	}
 
 }

tests/phpunit/unit/ReferencesFormatterTest.php

@@ -305,7 +305,7 @@ class ReferencesFormatterTest extends \MediaWikiUnitTestCase {
 		) );
 
 		$parser = $this->createNoOpMock( Parser::class );
-		$output = $formatter->referenceText( $parser, $key, $text, $isSectionPreview );
+		$output = $formatter->referenceText( $parser, $key, [ 'text' => $text ], $isSectionPreview );
 		$this->assertSame( $expectedOutput, $output );
 	}