mirror of
https://gerrit.wikimedia.org/r/mediawiki/extensions/Cite
synced 2024-09-24 02:38:57 +00:00
[Refactor] Pass validation error with StatusValue
This has clearer semantics than checking for a `false` attribute. Change-Id: I68f777eda40f8f157deafacaed02d4bd10cbf25c
This commit is contained in:
parent
99ee9e443b
commit
22a0350d84
45
src/Cite.php
45
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 <ref no_valid_attr="foo" />
|
||||
// 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 <ref />; 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 <ref>, but still display it, along with an error
|
||||
return array_fill_keys( $allowedAttributes, false );
|
||||
if ( count( $allValues ) > $maxCount ) {
|
||||
// A <ref> must have a name (can be null), but <references> 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 );
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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 ] ],
|
||||
|
|
Loading…
Reference in a new issue