diff --git a/src/Cite.php b/src/Cite.php index 4fdcd7e1e..261beadb2 100644 --- a/src/Cite.php +++ b/src/Cite.php @@ -266,13 +266,6 @@ class Cite { return StatusValue::newFatal( 'cite_error_ref_no_input' ); } - if ( $name === false ) { - // Invalid attribute in the tag like - // or name and follow attribute used both in one tag - // TODO: Move validation out of parseArguments - return StatusValue::newFatal( 'cite_error_ref_too_many_keys' ); - } - if ( $text === null && $name === null ) { // Something like ; this makes no sense. // TODO: Is this redundant with no_input? @@ -320,7 +313,7 @@ class Cite { $parser->getOutput()->setProperty( self::BOOK_REF_PROPERTY, true ); } - $result = $this->parseArguments( + $status = $this->parseArguments( $argv, [ 'dir', self::BOOK_REF_ATTRIBUTE, 'follow', 'group', 'name' ] ); @@ -330,14 +323,14 @@ class Cite { 'follow' => $follow, 'group' => $group, 'name' => $name - ] = $result; + ] = $status->getValue(); - # Split these into groups. + // Use the default group, or the references group when inside one. if ( $group === null ) { $group = $this->inReferencesGroup ?? self::DEFAULT_GROUP; } - $status = $this->validateRef( $text, $name, $group, $follow, $extends ); + $status->merge( $this->validateRef( $text, $name, $group, $follow, $extends ) ); if ( $this->inReferencesGroup !== null ) { if ( !$status->isOK() ) { @@ -400,18 +393,23 @@ class Cite { * @param string[] $argv The argument vector * @param string[] $allowedAttributes Allowed attribute names * - * @return (string|false|null)[] An array with exactly five elements, where each is a string on - * valid input, false on invalid input, or null on no input. + * @return StatusValue Either an error, or has a value with the dictionary of field names and + * parsed or default values. Missing attributes will be `null`. */ private function parseArguments( array $argv, array $allowedAttributes ) { - $defaultValues = array_fill_keys( $allowedAttributes, null ); + $maxCount = count( $allowedAttributes ); + $allValues = array_merge( array_fill_keys( $allowedAttributes, null ), $argv ); + $status = StatusValue::newGood( array_slice( $allValues, 0, $maxCount ) ); - if ( array_diff_key( $argv, $defaultValues ) ) { - // FIXME: This shouldn't kill the , but still display it, along with an error - return array_fill_keys( $allowedAttributes, false ); + if ( count( $allValues ) > $maxCount ) { + // A must have a name (can be null), but can't have one + $status->fatal( in_array( 'name', $allowedAttributes ) + ? 'cite_error_ref_too_many_keys' + : 'cite_error_references_invalid_parameters' + ); } - return array_merge( $defaultValues, $argv ); + return $status; } /** @@ -451,11 +449,11 @@ class Cite { ) { global $wgCiteResponsiveReferences; - $result = $this->parseArguments( + $status = $this->parseArguments( $argv, [ 'group', 'responsive' ] ); - [ 'group' => $group, 'responsive' => $responsive ] = $result; + [ 'group' => $group, 'responsive' => $responsive ] = $status->getValue(); $this->inReferencesGroup = $group ?? self::DEFAULT_GROUP; if ( strval( $text ) !== '' ) { @@ -490,9 +488,10 @@ class Cite { $responsive = $wgCiteResponsiveReferences; } - // There are remaining parameters we don't recognise - if ( $group === false ) { - return $this->errorReporter->halfParsed( 'cite_error_references_invalid_parameters' ); + if ( !$status->isOK() ) { + // Bail out with an error. + $error = $status->getErrors()[0]; + return $this->errorReporter->halfParsed( $error['message'], ...$error['params'] ); } $s = $this->referencesFormat( $this->inReferencesGroup, $responsive ); diff --git a/tests/parser/bookReferencing.txt b/tests/parser/bookReferencing.txt index 52a9e336f..2758c2fb7 100644 --- a/tests/parser/bookReferencing.txt +++ b/tests/parser/bookReferencing.txt @@ -406,7 +406,7 @@ wgCiteBookReferencing=true # TODO: # * Should render an error at the second reference. !! test -T236256 - Cant be a follow up and a book reference the same time +T236256 - Can't be a follow up and a book reference the same time !! config wgCiteBookReferencing=true !! wikitext diff --git a/tests/phpunit/unit/CiteUnitTest.php b/tests/phpunit/unit/CiteUnitTest.php index 55261336d..c964d906b 100644 --- a/tests/phpunit/unit/CiteUnitTest.php +++ b/tests/phpunit/unit/CiteUnitTest.php @@ -103,14 +103,22 @@ class CiteUnitTest extends \MediaWikiUnitTestCase { * @covers ::parseArguments * @dataProvider provideRefAttributes */ - public function testRefArg( array $attributes, array $expected ) { + public function testParseRefAttributes( + array $attributes, + array $expectedValue, + string $expectedError = null + ) { /** @var Cite $cite */ $cite = TestingAccessWrapper::newFromObject( new Cite() ); - $result = $cite->parseArguments( + $status = $cite->parseArguments( $attributes, [ 'dir', 'extends', 'follow', 'group', 'name' ] ); - $this->assertSame( $expected, array_values( $result ) ); + $this->assertSame( $expectedValue, array_values( $status->getValue() ) ); + $this->assertSame( !$expectedError, $status->isOK() ); + if ( $expectedError ) { + $this->assertSame( $expectedError, $status->getErrors()[0]['message'] ); + } } public function provideRefAttributes() { @@ -124,8 +132,16 @@ class CiteUnitTest extends \MediaWikiUnitTestCase { [ [ 'dir' => 'rtl' ], [ 'rtl', null, null, null, null ] ], [ [ 'follow' => 'f' ], [ null, null, 'f', null, null ] ], [ [ 'group' => 'g' ], [ null, null, null, 'g', null ] ], - [ [ 'invalid' => 'i' ], [ false, false, false, false, false ] ], - [ [ 'invalid' => null ], [ false, false, false, false, false ] ], + [ + [ 'invalid' => 'i' ], + [ null, null, null, null, null ], + 'cite_error_ref_too_many_keys' + ], + [ + [ 'invalid' => null ], + [ null, null, null, null, null ], + 'cite_error_ref_too_many_keys' + ], [ [ 'name' => 'n' ], [ null, null, null, null, 'n' ] ], [ [ 'name' => null ], [ null, null, null, null, null ] ], [ [ 'extends' => 'e' ], [ null, 'e', null, null, null ] ],