Final clean-ups for a more consistent parameter order

* Always have an empty line between @param and @return to improve
readability as well as consistency within this codebase (before, both
styles have been used).

* Flip parameter order in validateRefInReferences() for consistency with
the rest of the code.

* In Cite::guardedRef() the Parser was now the 1st parameter. I changed
all related functions the same way to make the code less surprising.

* Same in CiteUnitTest. This is really just the @dataProvider. But I feel
it's still helpful to have the arguments in the same order everywhere, if
possible.

* Add a few strict type hints.

* It seems the preferred style for PHP7 return types is `… ) : string {`
with a space before the `:`. There is currently no PHPCS sniff for this.
However, I think this codebase should be consistent, one way or the other.

Change-Id: I91d232be727afd26ff20526ab4ef63aa5ba6bacf

src/Cite.php

@@ -119,13 +119,13 @@ class Cite {
 	/**
 	 * Callback function for <ref>
 	 *
-	 * @param string|null $text Raw content of the <ref> tag.
-	 * @param string[] $argv Arguments
 	 * @param Parser $parser
+	 * @param ?string $text Raw, untrimmed wikitext content of the <ref> tag, if any
+	 * @param string[] $argv Arguments as given in <ref name=…>, already trimmed
 	 *
 	 * @return string|false False in case a <ref> tag is not allowed in the current context
 	 */
-	public function ref( ?string $text, array $argv, Parser $parser ) {
+	public function ref( Parser $parser, ?string $text, array $argv ) {
 		if ( $this->mInCite ) {
 			return false;
 		}
@@ -191,13 +191,13 @@ class Cite {
 
 		return $this->inReferencesGroup === null ?
 			$this->validateRefOutsideOfReferences( $text, $name ) :
-			$this->validateRefInReferences( $text, $name, $group );
+			$this->validateRefInReferences( $text, $group, $name );
 	}
 
 	private function validateRefOutsideOfReferences(
 		?string $text,
 		?string $name
-	): StatusValue {
+	) : StatusValue {
 		if ( !$name ) {
 			if ( $text === null ) {
 				// Something like <ref />; this makes no sense.
@@ -233,9 +233,9 @@ class Cite {
 
 	private function validateRefInReferences(
 		?string $text,
-		?string $name,
+		string $group,
-		string $group
+		?string $name
-	): StatusValue {
+	) : StatusValue {
 		// FIXME: Some assertions make assumptions that rely on earlier tests not failing.
 		//  These dependencies need to be explicit so they aren't accidentally broken by
 		//  reordering in the future, or made more robust to initial conditions.
@@ -284,8 +284,8 @@ class Cite {
 	 *  special handling for each context.
 	 *
 	 * @param Parser $parser
-	 * @param string|null $text Raw content of the <ref> tag.
+	 * @param ?string $text Raw, untrimmed wikitext content of the <ref> tag, if any
-	 * @param string[] $argv Arguments
+	 * @param string[] $argv Arguments as given in <ref name=…>, already trimmed
 	 *
 	 * @return string HTML
 	 */
@@ -310,8 +310,6 @@ class Cite {
 			$arguments['group'] = $this->inReferencesGroup ?? self::DEFAULT_GROUP;
 		}
 
-		[ 'group' => $group, 'name' => $name ] = $arguments;
-
 		$status->merge( $this->validateRef( $text, ...array_values( $arguments ) ) );
 
 		// Validation cares about the difference between null and empty, but from here on we don't
@@ -319,6 +317,8 @@ class Cite {
 			$text = null;
 		}
 
+		[ 'group' => $group, 'name' => $name ] = $arguments;
+
 		if ( $this->inReferencesGroup !== null ) {
 			if ( !$status->isOK() ) {
 				foreach ( $status->getErrors() as $error ) {
@@ -394,18 +394,18 @@ class Cite {
 	/**
 	 * Callback function for <references>
 	 *
-	 * @param string|null $text Raw content of the <references> tag.
-	 * @param string[] $argv Arguments
 	 * @param Parser $parser
+	 * @param ?string $text Raw, untrimmed wikitext content of the <references> tag, if any
+	 * @param string[] $argv Arguments as given in <references name=…>, already trimmed
 	 *
 	 * @return string|false False in case a <references> tag is not allowed in the current context
 	 */
-	public function references( ?string $text, array $argv, Parser $parser ) {
+	public function references( Parser $parser, ?string $text, array $argv ) {
 		if ( $this->mInCite || $this->inReferencesGroup !== null ) {
 			return false;
 		}
 
-		$ret = $this->guardedReferences( $text, $argv, $parser );
+		$ret = $this->guardedReferences( $parser, $text, $argv );
 		$this->inReferencesGroup = null;
 
 		return $ret;
@@ -414,16 +414,16 @@ class Cite {
 	/**
 	 * Must only be called from references(). Use that to prevent recursion.
 	 *
-	 * @param string|null $text Raw content of the <references> tag.
-	 * @param string[] $argv
 	 * @param Parser $parser
+	 * @param ?string $text Raw, untrimmed wikitext content of the <references> tag, if any
+	 * @param string[] $argv Arguments as given in <references name=…>, already trimmed
 	 *
 	 * @return string HTML
 	 */
 	private function guardedReferences(
+		Parser $parser,
 		?string $text,
-		array $argv,
+		array $argv
-		Parser $parser
 	) : string {
 		$status = $this->parseArguments( $argv, [ 'group', 'responsive' ] );
 		[ 'group' => $group, 'responsive' => $responsive ] = $status->getValue();
@@ -439,9 +439,7 @@ class Cite {
 			$count = substr_count( $text, Parser::MARKER_PREFIX . "-ref-" );
 
 			// Undo effects of calling <ref> while unaware of being contained in <references>
-			$redoStack = $this->referenceStack->rollbackRefs( $count );
+			foreach ( $this->referenceStack->rollbackRefs( $count ) as $call ) {
-
-			foreach ( $redoStack as $call ) {
 				// Rerun <ref> call with the <references> context now being known
 				$this->guardedRef( $parser, ...$call );
 			}

src/ErrorReporter.php

@@ -86,7 +86,7 @@ class ErrorReporter {
 	 *
 	 * @return Language
 	 */
-	private function getInterfaceLanguageAndSplitCache( Parser $parser ): Language {
+	private function getInterfaceLanguageAndSplitCache( Parser $parser ) : Language {
 		if ( !$this->cachedInterfaceLanguage ) {
 			$this->cachedInterfaceLanguage = $parser->getOptions()->getUserLangObj();
 		}

src/Hooks/CiteParserTagHooks.php

@@ -24,19 +24,24 @@ class CiteParserTagHooks {
 	/**
 	 * Parser hook for the <ref> tag.
 	 *
-	 * @param string|null $content Raw wikitext content of the <ref> tag.
+	 * @param ?string $text Raw, untrimmed wikitext content of the <ref> tag, if any
-	 * @param string[] $attributes
+	 * @param string[] $argv
 	 * @param Parser $parser
 	 * @param PPFrame $frame
 	 *
 	 * @return string HTML
 	 */
-	public static function ref( $content, array $attributes, Parser $parser, PPFrame $frame ) {
+	public static function ref(
+		?string $text,
+		array $argv,
+		Parser $parser,
+		PPFrame $frame
+	) : string {
 		$cite = self::citeForParser( $parser );
-		$result = $cite->ref( $content, $attributes, $parser );
+		$result = $cite->ref( $parser, $text, $argv );
 
 		if ( $result === false ) {
-			return htmlspecialchars( "<ref>$content</ref>" );
+			return htmlspecialchars( "<ref>$text</ref>" );
 		}
 
 		$parserOutput = $parser->getOutput();
@@ -51,21 +56,26 @@ class CiteParserTagHooks {
 	/**
 	 * Parser hook for the <references> tag.
 	 *
-	 * @param string|null $content Raw wikitext content of the <references> tag.
+	 * @param ?string $text Raw, untrimmed wikitext content of the <references> tag, if any
-	 * @param string[] $attributes
+	 * @param string[] $argv
 	 * @param Parser $parser
 	 * @param PPFrame $frame
 	 *
 	 * @return string HTML
 	 */
-	public static function references( $content, array $attributes, Parser $parser, PPFrame $frame ) {
+	public static function references(
+		?string $text,
+		array $argv,
+		Parser $parser,
+		PPFrame $frame
+	) : string {
 		$cite = self::citeForParser( $parser );
-		$result = $cite->references( $content, $attributes, $parser );
+		$result = $cite->references( $parser, $text, $argv );
 
 		if ( $result === false ) {
-			return htmlspecialchars( $content === null
+			return htmlspecialchars( $text === null
 				? "<references/>"
-				: "<references>$content</references>"
+				: "<references>$text</references>"
 			);
 		}
 
@@ -81,7 +91,7 @@ class CiteParserTagHooks {
 	 *
 	 * @return Cite
 	 */
-	private static function citeForParser( Parser $parser ): Cite {
+	private static function citeForParser( Parser $parser ) : Cite {
 		if ( !isset( $parser->extCite ) ) {
 			$parser->extCite = new Cite( $parser );
 		}

src/ReferenceMessageLocalizer.php

@@ -31,7 +31,7 @@ class ReferenceMessageLocalizer implements MessageLocalizer {
 	 *
 	 * @return string
 	 */
-	public function formatNum( string $number ): string {
+	public function formatNum( string $number ) : string {
 		return $this->language->formatNum( $number );
 	}
 
@@ -42,7 +42,7 @@ class ReferenceMessageLocalizer implements MessageLocalizer {
 	 *
 	 * @return string
 	 */
-	public function localizeDigits( string $number ): string {
+	public function localizeDigits( string $number ) : string {
 		return $this->language->formatNumNoSeparators( $number );
 	}
 
@@ -61,7 +61,7 @@ class ReferenceMessageLocalizer implements MessageLocalizer {
 	 *
 	 * @return Message
 	 */
-	public function msg( $key, ...$params ): Message {
+	public function msg( $key, ...$params ) : Message {
 		return wfMessage( $key, ...$params )->inLanguage( $this->language );
 	}
 

src/ReferenceStack.php

@@ -369,7 +369,7 @@ class ReferenceStack {
 	 *
 	 * @return array[] The references from the removed group
 	 */
-	public function popGroup( string $group ) {
+	public function popGroup( string $group ) : array {
 		$refs = $this->getGroupRefs( $group );
 		unset( $this->refs[$group] );
 		unset( $this->groupRefSequence[$group] );

tests/phpunit/CiteDbTest.php

@@ -46,7 +46,7 @@ class CiteDbTest extends \MediaWikiIntegrationTestCase {
 		);
 	}
 
-	private function newCite(): Cite {
+	private function newCite() : Cite {
 		$mockOptions = $this->createMock( ParserOptions::class );
 		$mockOptions->method( 'getIsPreview' )->willReturn( false );
 		$mockOptions->method( 'getIsSectionPreview' )->willReturn( false );

tests/phpunit/CiteIntegrationTest.php

@@ -120,7 +120,7 @@ class CiteIntegrationTest extends \MediaWikiIntegrationTestCase {
 		];
 	}
 
-	private function newCite(): Cite {
+	private function newCite() : Cite {
 		$mockOptions = $this->createMock( ParserOptions::class );
 		$mockOptions->method( 'getIsPreview' )->willReturn( false );
 		$mockOptions->method( 'getIsSectionPreview' )->willReturn( false );

tests/phpunit/unit/CiteDataModuleTest.php

@@ -48,10 +48,7 @@ class CiteDataModuleTest extends \MediaWikiUnitTestCase {
 		);
 	}
 
-	/**
+	private function createResourceLoaderContext() : ResourceLoaderContext {
-	 * @return ResourceLoaderContext
-	 */
-	private function createResourceLoaderContext() {
 		$msg = $this->createMock( Message::class );
 		$msg->method( 'inContentLanguage' )
 			->willReturnSelf();

tests/phpunit/unit/CiteUnitTest.php

@@ -32,10 +32,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 		?string $inReferencesGroup,
 		bool $isSectionPreview,
 		?string $text,
-		?string $name,
 		?string $group,
-		?string $follow,
+		?string $name,
 		?string $extends,
+		?string $follow,
 		?string $dir,
 		$expected
 	) {
@@ -67,10 +67,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => null,
-				'name' => '1',
 				'group' => '',
-				'follow' => null,
+				'name' => '1',
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_ref_numeric_key',
 			],
@@ -79,10 +79,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 't',
-				'name' => null,
 				'group' => '',
-				'follow' => '1',
+				'name' => null,
 				'extends' => null,
+				'follow' => '1',
 				'dir' => null,
 				'expected' => 'cite_error_ref_numeric_key',
 			],
@@ -91,10 +91,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 't',
-				'name' => null,
 				'group' => '',
-				'follow' => null,
+				'name' => null,
 				'extends' => '1',
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_ref_numeric_key',
 			],
@@ -103,10 +103,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 't',
-				'name' => 'n',
 				'group' => '',
-				'follow' => 'f',
+				'name' => 'n',
 				'extends' => null,
+				'follow' => 'f',
 				'dir' => null,
 				'expected' => 'cite_error_ref_too_many_keys',
 			],
@@ -115,10 +115,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 't',
-				'name' => null,
 				'group' => '',
-				'follow' => 'f',
+				'name' => null,
 				'extends' => 'e',
+				'follow' => 'f',
 				'dir' => null,
 				'expected' => 'cite_error_ref_too_many_keys',
 			],
@@ -128,10 +128,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 't',
-				'name' => null,
 				'group' => '',
-				'follow' => null,
+				'name' => null,
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => true,
 			],
@@ -140,10 +140,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => '',
-				'name' => null,
 				'group' => '',
-				'follow' => null,
+				'name' => null,
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_ref_no_input',
 			],
@@ -152,10 +152,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => null,
-				'name' => null,
 				'group' => '',
-				'follow' => null,
+				'name' => null,
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_ref_no_key',
 			],
@@ -164,10 +164,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 'Foo <ref name="bar">',
-				'name' => 'n',
 				'group' => '',
-				'follow' => null,
+				'name' => 'n',
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_included_ref',
 			],
@@ -178,10 +178,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => 'g',
 				'isSectionPreview' => false,
 				'text' => 'not empty',
-				'name' => 'n',
 				'group' => 'g',
-				'follow' => null,
+				'name' => 'n',
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => true,
 			],
@@ -190,10 +190,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => 'g1',
 				'isSectionPreview' => false,
 				'text' => 't',
-				'name' => 'n',
 				'group' => 'g2',
-				'follow' => null,
+				'name' => 'n',
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_references_group_mismatch',
 			],
@@ -202,10 +202,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => 'g',
 				'isSectionPreview' => false,
 				'text' => 't',
-				'name' => null,
 				'group' => 'g',
-				'follow' => null,
+				'name' => null,
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_references_no_key',
 			],
@@ -214,10 +214,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => 'g',
 				'isSectionPreview' => false,
 				'text' => '',
-				'name' => 'n',
 				'group' => 'g',
-				'follow' => null,
+				'name' => 'n',
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_empty_references_define',
 			],
@@ -226,10 +226,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => 'g',
 				'isSectionPreview' => false,
 				'text' => 'not empty',
-				'name' => 'n',
 				'group' => 'g',
-				'follow' => null,
+				'name' => 'n',
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_references_missing_group',
 			],
@@ -238,10 +238,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => 'g',
 				'isSectionPreview' => false,
 				'text' => 'not empty',
-				'name' => 'n2',
 				'group' => 'g',
-				'follow' => null,
+				'name' => 'n2',
 				'extends' => null,
+				'follow' => null,
 				'dir' => null,
 				'expected' => 'cite_error_references_missing_key',
 			],
@@ -250,10 +250,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 'not empty',
-				'name' => 'n',
 				'group' => '',
-				'follow' => null,
+				'name' => 'n',
 				'extends' => null,
+				'follow' => null,
 				'dir' => 'RTL',
 				'expected' => true,
 			],
@@ -262,10 +262,10 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 				'inReferencesGroup' => null,
 				'isSectionPreview' => false,
 				'text' => 'not empty',
-				'name' => 'n',
 				'group' => '',
-				'follow' => null,
+				'name' => 'n',
 				'extends' => null,
+				'follow' => null,
 				'dir' => 'foobar',
 				'expected' => 'cite_error_ref_invalid_dir',
 			],
@@ -386,7 +386,7 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 			->with( $expectedRollbackCount )
 			->willReturn( [ [ 't', [] ] ] );
 
-		$output = $spy->guardedReferences( $text, $argv, $parser );
+		$output = $spy->guardedReferences( $parser, $text, $argv );
 		$this->assertSame( $expectedOutput, $output );
 	}
 
@@ -661,7 +661,7 @@ class CiteUnitTest extends \MediaWikiUnitTestCase {
 		$this->assertNotSame( $clone->referenceStack, $spy->referenceStack );
 	}
 
-	private function newCite(): Cite {
+	private function newCite() : Cite {
 		$mockOptions = $this->createMock( ParserOptions::class );
 		$mockOptions->method( 'getIsPreview' )->willReturn( false );
 		$mockOptions->method( 'getIsSectionPreview' )->willReturn( false );