Rewrite ErrorReporter for performance and separation of concerns

This patch is mostly moving code around without changing the behavior.
Exceptions:

* The ErrorReporter creates a <span> container. This was previously
parsed. The only benefit might be error checking and escaping. Rather
pointless. The code just created this HTML. With this patch, it is not
parsed any more. The unit test reflects this change. The output in
production will not change, as the parser tests show.

* Parsing of the message key (to detect it's type and id) is simplified
a lot, using explode. With this the code can, in theory, support more
types.

Bug: T239572
Change-Id: If2fe5f55db46dfc7e0ce445348608bef00bec64e

src/ErrorReporter.php

@@ -4,6 +4,7 @@ namespace Cite;
 
 use Html;
 use Language;
+use Message;
 use Parser;
 use Sanitizer;
 
@@ -25,9 +26,7 @@ class ErrorReporter {
 	/**
 	 * @param ReferenceMessageLocalizer $messageLocalizer
 	 */
-	public function __construct(
+	public function __construct( ReferenceMessageLocalizer $messageLocalizer ) {
-		ReferenceMessageLocalizer $messageLocalizer
-	) {
 		$this->messageLocalizer = $messageLocalizer;
 	}
 
@@ -39,8 +38,9 @@ class ErrorReporter {
 	 * @return string Half-parsed wikitext with extension's tags already being expanded
 	 */
 	public function halfParsed( Parser $parser, string $key, ...$params ) : string {
-		// FIXME: We suspect this is not necessary and can just be removed
+		$msg = $this->msg( $parser, $key, ...$params );
-		return $parser->recursiveTagParse( $this->plain( $parser, $key, ...$params ) );
+		$wikitext = $parser->recursiveTagParse( $msg->plain() );
+		return $this->wrapInHtmlContainer( $wikitext, $key, $msg->getLanguage() );
 	}
 
 	/**
@@ -52,31 +52,31 @@ class ErrorReporter {
 	 * @return-taint tainted
 	 */
 	public function plain( Parser $parser, string $key, ...$params ) : string {
-		$interfaceLanguage = $this->getInterfaceLanguageAndSplitCache( $parser );
+		$msg = $this->msg( $parser, $key, ...$params );
-		$msg = $this->messageLocalizer->msg( $key, ...$params )->inLanguage( $interfaceLanguage );
+		$wikitext = $msg->plain();
+		return $this->wrapInHtmlContainer( $wikitext, $key, $msg->getLanguage() );
+	}
 
-		if ( strncmp( $msg->getKey(), 'cite_warning_', 13 ) === 0 ) {
+	/**
-			$type = 'warning';
+	 * @param Parser $parser
-			$id = substr( $msg->getKey(), 13 );
+	 * @param string $key
-			$extraClass = ' mw-ext-cite-warning-' . Sanitizer::escapeClass( $id );
+	 * @param mixed ...$params
-		} else {
+	 *
-			$type = 'error';
+	 * @return Message
-			$extraClass = '';
+	 */
+	private function msg( Parser $parser, string $key, ...$params ) : Message {
+		$language = $this->getInterfaceLanguageAndSplitCache( $parser );
+		$msg = $this->messageLocalizer->msg( $key, ...$params )->inLanguage( $language );
 
+		[ $type, ] = $this->parseTypeAndIdFromMessageKey( $msg->getKey() );
+
+		if ( $type === 'error' ) {
 			// Take care; this is a sideeffect that might not belong to this class.
 			$parser->addTrackingCategory( 'cite-tracking-category-cite-error' );
 		}
 
-		return Html::rawElement(
+		// Messages: cite_error, cite_warning
-			'span',
+		return $this->messageLocalizer->msg( "cite_$type", $msg->plain() )->inLanguage( $language );
-			[
-				'class' => "$type mw-ext-cite-$type" . $extraClass,
-				'lang' => $interfaceLanguage->getHtmlCode(),
-				'dir' => $interfaceLanguage->getDir(),
-			],
-			$this->messageLocalizer->msg( "cite_$type", $msg->plain() )
-				->inLanguage( $interfaceLanguage )->plain()
-		);
 	}
 
 	/**
@@ -93,4 +93,41 @@ class ErrorReporter {
 		return $this->cachedInterfaceLanguage;
 	}
 
+	/**
+	 * @param string $wikitext
+	 * @param string $key
+	 * @param Language $language
+	 *
+	 * @return string
+	 */
+	private function wrapInHtmlContainer(
+		string $wikitext,
+		string $key,
+		Language $language
+	) : string {
+		[ $type, $id ] = $this->parseTypeAndIdFromMessageKey( $key );
+		$extraClass = $type === 'warning'
+			? ' mw-ext-cite-warning-' . Sanitizer::escapeClass( $id )
+			: '';
+
+		return Html::rawElement(
+			'span',
+			[
+				'class' => "$type mw-ext-cite-$type" . $extraClass,
+				'lang' => $language->getHtmlCode(),
+				'dir' => $language->getDir(),
+			],
+			$wikitext
+		);
+	}
+
+	/**
+	 * @param string $messageKey Expected to be a message key like "cite_error_ref_too_many_keys"
+	 *
+	 * @return string[]
+	 */
+	private function parseTypeAndIdFromMessageKey( string $messageKey ) : array {
+		return array_slice( explode( '_', $messageKey, 3 ), 1 );
+	}
+
 }

tests/phpunit/unit/ErrorReporterTest.php

@@ -27,8 +27,9 @@ class ErrorReporterTest extends \MediaWikiUnitTestCase {
 		string $expectedHtml,
 		array $expectedCategories
 	) {
-		$reporter = $this->createReporter();
+		$language = $this->createLanguage();
-		$mockParser = $this->createParser( $expectedCategories );
+		$reporter = $this->createReporter( $language );
+		$mockParser = $this->createParser( $language, $expectedCategories );
 		$this->assertSame(
 			$expectedHtml,
 			$reporter->plain( $mockParser, $key, 'first param' ) );
@@ -38,11 +39,12 @@ class ErrorReporterTest extends \MediaWikiUnitTestCase {
 	 * @covers ::halfParsed
 	 */
 	public function testHalfParsed() {
-		$reporter = $this->createReporter();
+		$language = $this->createLanguage();
-		$mockParser = $this->createParser( [] );
+		$reporter = $this->createReporter( $language );
+		$mockParser = $this->createParser( $language, [] );
 		$this->assertSame(
-			'[<span class="warning mw-ext-cite-warning mw-ext-cite-warning-example" lang="qqx" ' .
+			'<span class="warning mw-ext-cite-warning mw-ext-cite-warning-example" lang="qqx" ' .
-				'dir="rtl">(cite_warning|(cite_warning_example|first param))</span>]',
+				'dir="rtl">[(cite_warning|(cite_warning_example|first param))]</span>',
 			$reporter->halfParsed( $mockParser, 'cite_warning_example', 'first param' ) );
 	}
 
@@ -63,16 +65,22 @@ class ErrorReporterTest extends \MediaWikiUnitTestCase {
 		];
 	}
 
-	private function createReporter() : ErrorReporter {
+	private function createLanguage() : Language {
+		$language = $this->createMock( Language::class );
+		$language->method( 'getDir' )->willReturn( 'rtl' );
+		$language->method( 'getHtmlCode' )->willReturn( 'qqx' );
+		return $language;
+	}
+
+	private function createReporter( Language $language ) : ErrorReporter {
 		$mockMessageLocalizer = $this->createMock( ReferenceMessageLocalizer::class );
 		$mockMessageLocalizer->method( 'msg' )->willReturnCallback(
-			function ( ...$args ) {
+			function ( ...$args ) use ( $language ) {
 				$message = $this->createMock( Message::class );
 				$message->method( 'getKey' )->willReturn( $args[0] );
 				$message->method( 'plain' )->willReturn( '(' . implode( '|', $args ) . ')' );
-				$message->method( 'inLanguage' )->with( $this->callback( function ( Language $lang ) {
+				$message->method( 'inLanguage' )->with( $language )->willReturnSelf();
-					return $lang->getHtmlCode() === 'qqx';
+				$message->method( 'getLanguage' )->willReturn( $language );
-				} ) )->willReturnSelf();
 				return $message;
 			}
 		);
@@ -81,11 +89,7 @@ class ErrorReporterTest extends \MediaWikiUnitTestCase {
 		return new ErrorReporter( $mockMessageLocalizer );
 	}
 
-	public function createParser( array $expectedCategories ) {
+	public function createParser( Language $language, array $expectedCategories ) {
-		$language = $this->createMock( Language::class );
-		$language->method( 'getDir' )->willReturn( 'rtl' );
-		$language->method( 'getHtmlCode' )->willReturn( 'qqx' );
-
 		$parserOptions = $this->createMock( ParserOptions::class );
 		$parserOptions->method( 'getUserLangObj' )->willReturn( $language );