jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/360871 )
Change subject: Introduce more specific API error messages and allow
localization
......................................................................
Introduce more specific API error messages and allow localization
This patch focuses on a few error messages that got lost in Ib5b1eae.
I'm either introducing new localizable messages for common use cases,
or reuse existing ones.
This also fixes a long standing bug in ChangeOpStatement. This is very
closely related to the other changes because it is also about properly
reporting API errors with localizable messages.
The only minor detail that might be a little bit controversial are the
two places where I reuse the existing "wikibase-api-invalid-property-id"
message. This does not have a parameter. The following patch I0d2a471
will introduce that parameter.
Bug: T168859
Change-Id: I5c26fc968fd800a9df8e65f1cf7e099fd3644648
---
M repo/i18n/en.json
M repo/i18n/qqq.json
M repo/includes/Api/CreateClaim.php
M repo/includes/Api/EditEntity.php
M repo/includes/Api/ModifyEntity.php
M repo/includes/Api/ParseValue.php
M repo/includes/Api/SetQualifier.php
M repo/includes/ChangeOp/ChangeOpStatement.php
M repo/tests/phpunit/includes/Api/EditEntityTest.php
M repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
M repo/tests/phpunit/includes/Api/IndependentWikibaseApiTestCase.php
M repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
M repo/tests/phpunit/includes/ChangeOp/ChangeOpStatementTest.php
13 files changed, 81 insertions(+), 47 deletions(-)
Approvals:
Jonas Kress (WMDE): Looks good to me, approved
jenkins-bot: Verified
diff --git a/repo/i18n/en.json b/repo/i18n/en.json
index 10d109c..0254e04 100644
--- a/repo/i18n/en.json
+++ b/repo/i18n/en.json
@@ -327,6 +327,8 @@
"wikibase-api-failed-add-sitelink": "The sitelink could not be saved.",
"wikibase-api-failed-modify": "Attempted modification of the item
failed.",
"wikibase-api-failed-save": "The save has failed.",
+ "wikibase-api-illegal-entity-remove": "Entities can not be deleted by
providing a top-level <var>remove</var> key.",
+ "wikibase-api-illegal-entity-selector": "You need to provide either an
entity <var>id</var>, or a <var>site</var> and <var>page</var> combination, but
not both.",
"wikibase-api-inconsistent-language": "Inconsistent language detected.",
"wikibase-api-inconsistent-site": "Inconsistent site detected.",
"wikibase-api-invalid-guid": "Invalid claim guid.",
@@ -341,7 +343,7 @@
"wikibase-api-nosuchrevid": "Revision with ID not found.",
"wikibase-api-no-such-claim": "Could not find such a claim.",
"wikibase-api-no-such-entity": "Could not find such an entity.",
- "wikibase-api-no-such-entity-link": "Could not find such an entity
link.",
+ "wikibase-api-no-such-entity-link": "Could not find an item containing
a sitelink to the provided site and page name.",
"wikibase-api-no-such-reference": "Could not find such a reference.",
"wikibase-api-no-such-site": "Could not find such a site.",
"wikibase-api-no-such-sitelink": "Could not find a sitelink to \"$1\"
when trying to edit badges.",
@@ -349,6 +351,7 @@
"wikibase-api-not-supported": "The requested feature is not supported
by the given entity.",
"wikibase-api-not-statement": "Not a statement.",
"wikibase-api-not-item": "Not an item.",
+ "wikibase-api-not-recognized-datatype": "A datatype was expected, but
either missing or not recognized.",
"wikibase-api-not-recognized-siteid": "The supplied site identifier was
not recognized.",
"wikibase-api-not-recognized": "Something was not recognized.",
"wikibase-api-not-recognized-array": "An array was expected, but not
recognized.",
diff --git a/repo/i18n/qqq.json b/repo/i18n/qqq.json
index 81b4d49..ca45b27 100644
--- a/repo/i18n/qqq.json
+++ b/repo/i18n/qqq.json
@@ -359,13 +359,15 @@
"wikibase-api-failed-add-sitelink": "This is an error message where the
system of some unspecific reason could not add the sitelink to the temporary
store.\n\nSee also:\n* {{msg-mw|Wikibase-newitem-add-sitelink-failed}}",
"wikibase-api-failed-modify": "!!DO NOT TRANSLATE!! This is an error
message for a situation where the API tries to modify an item but the operation
cannot be completed. Usually this should never be shown to the user, unless
there are some exceptional error conditions, or the database is in maintenance
mode.",
"wikibase-api-failed-save": "!!DO NOT TRANSLATE!! This is an error
message for a situation where the API tries to save an item but the operation
cannot be completed.",
+ "wikibase-api-illegal-entity-remove": "Error message when the user
misplaced a <var>remove</var> key too high in the JSON structure they
provided.",
+ "wikibase-api-illegal-entity-selector": "Error message when the user
fails to provide one of the parameter combinations allowed to address
entities.",
"wikibase-api-inconsistent-language": "!!DO NOT TRANSLATE!!
Inconsistent language detected",
"wikibase-api-inconsistent-site": "!!DO NOT TRANSLATE!! Inconsistent
site detected",
"wikibase-api-invalid-guid": "!!DO NOT TRANSLATE!! Invalid uniquie
identifying provided for a claim",
"wikibase-api-invalid-json": "!!DO NOT TRANSLATE!! The supplied JSON
structure is invalid for some reason, usually because it is malformed or
because it cannot be recreated in a legal form. The exact reason is not clear
and no attempt should be made to guess a reason.",
"wikibase-api-invalid-snak": "!!DO NOT TRANSLATE!! Invalid snak has
been provided, This is similar to invalid-json",
"wikibase-api-invalid-list": "!!DO NOT TRANSLATE!! Invalid list or data
has been provided. This can occour when a list conflicts with itself (the list
contains something to modify, but also to remove this item)",
- "wikibase-api-invalid-property-id": "!!DO NOT TRANSLATE!! Invalid
propertyid has been supplied",
+ "wikibase-api-invalid-property-id": "Error message when the user either
missed to provide a required property ID, or provided an ID that formally
belongs to an other entity type.",
"wikibase-api-invalid-entity-id": "!!DO NOT TRANSLATE!! Invalid
entityid has been supplied",
"wikibase-api-no-common-item": "!!DO NOT TRANSLATE!! There is no common
item between to passed parameters when there should be",
"wikibase-api-no-data": "!!DO NOT TRANSLATE!! This is an error message
for a situation where the \"data\" argument to the API is lacking content.
Usually this should never be shown to the user, unless there are some
exceptional error condition. This message should probably not exist in the
final version.",
@@ -373,7 +375,7 @@
"wikibase-api-nosuchrevid": "!!DO NOT TRANSLATE!! Page or entity with
this revision id could not be found in the database",
"wikibase-api-no-such-claim": "!!DO NOT TRANSLATE!! Could not find such
a claim on wikidata, this could be to the user entering the wrong data",
"wikibase-api-no-such-entity": "!!DO NOT TRANSLATE!! Could not find
such an entity, this could be to the user entering the wrong data",
- "wikibase-api-no-such-entity-link": "!!DO NOT TRANSLATE!! Could not
find such an entity link, this could be to the user entering the wrong data",
+ "wikibase-api-no-such-entity-link": "Error message when the user
provides a site and page name combination (usually referred to as a
\"sitelink\") not contained in any item.",
"wikibase-api-no-such-reference": "!!DO NOT TRANSLATE!! Could not find
such a reference, this could be to the user entering the wrong data",
"wikibase-api-no-such-site": "!!DO NOT TRANSLATE!! Could not find such
a site, this could be to the user entering the wrong data",
"wikibase-api-no-such-sitelink": "Could not find such a sitelink, this
could be to the user entering the wrong data\n* $1 - the site id",
@@ -381,6 +383,7 @@
"wikibase-api-not-supported": "!!DO NOT TRANSLATE!! The given entity
does not support the operation provided by the requested api module.",
"wikibase-api-not-statement": "!!DO NOT TRANSLATE!! The passed
parameter is Not a statement when a statement is expected",
"wikibase-api-not-item": "!!DO NOT TRANSLATE!! The passed parameter is
Not an item when an item is expected (the passed parameter may infact be a
property)",
+ "wikibase-api-not-recognized-datatype": "Error message when the user
failed to provide the name of a known data type.\n{{Identical|Data type}}",
"wikibase-api-not-recognized-siteid": "!!DO NOT TRANSLATE!! The
supplied site identifier was not recognized",
"wikibase-api-not-recognized": "!!DO NOT TRANSLATE!! Something was not
recognized (general message)",
"wikibase-api-not-recognized-array": "!!DO NOT TRANSLATE!! An array was
expected, but not recognized. The user probably passed the API a malformed
parameter",
diff --git a/repo/includes/Api/CreateClaim.php
b/repo/includes/Api/CreateClaim.php
index cb8a3f7..f98abc4 100644
--- a/repo/includes/Api/CreateClaim.php
+++ b/repo/includes/Api/CreateClaim.php
@@ -82,8 +82,8 @@
$propertyId = $this->modificationHelper->getEntityIdFromString(
$params['property'] );
if ( !( $propertyId instanceof PropertyId ) ) {
- $this->errorReporter->dieError(
- $propertyId->getSerialization() . ' does not
appear to be a property ID',
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-invalid-property-id',
'param-illegal'
);
}
diff --git a/repo/includes/Api/EditEntity.php b/repo/includes/Api/EditEntity.php
index ece500c..c37c5df 100644
--- a/repo/includes/Api/EditEntity.php
+++ b/repo/includes/Api/EditEntity.php
@@ -200,9 +200,8 @@
);
}
if ( $hasId && $hasSiteLink ) {
- $this->errorReporter->dieError(
- 'Parameter "id" and "site", "title" combination
are not allowed to be both set in'
- . ' the same request',
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-illegal-entity-selector',
'param-illegal'
);
}
@@ -251,13 +250,16 @@
// if we create a new property, make sure we set the datatype
if ( !$exists && $entity instanceof Property ) {
- if ( !isset( $data['datatype'] ) ) {
- $this->errorReporter->dieError( 'No datatype
given', 'param-illegal' );
- } elseif ( !in_array( $data['datatype'],
$this->propertyDataTypes ) ) {
- $this->errorReporter->dieError( 'Invalid
datatype given', 'param-illegal' );
- } else {
- $entity->setDataTypeId( $data['datatype'] );
+ if ( !isset( $data['datatype'] )
+ || !in_array( $data['datatype'],
$this->propertyDataTypes )
+ ) {
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-not-recognized-datatype',
+ 'param-illegal'
+ );
}
+
+ $entity->setDataTypeId( $data['datatype'] );
}
$changeOps = $this->getChangeOp( $data, $entity );
@@ -387,8 +389,8 @@
$this->assertString( $prop, 'Top level structure must
be a JSON object (no keys found)' );
if ( $prop === 'remove' ) {
- $this->errorReporter->dieError(
- '"remove" should not be a top-level
key',
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-illegal-entity-remove',
'not-recognized'
);
}
diff --git a/repo/includes/Api/ModifyEntity.php
b/repo/includes/Api/ModifyEntity.php
index ee3f05d..f5acd9e 100644
--- a/repo/includes/Api/ModifyEntity.php
+++ b/repo/includes/Api/ModifyEntity.php
@@ -207,8 +207,8 @@
if ( ( isset( $params['id'] ) || isset( $params['new'] ) )
=== ( isset( $params['site'] ) && isset(
$params['title'] ) )
) {
- $this->errorReporter->dieError(
- 'Either provide the item "id" or pairs of
"site" and "title" for a corresponding page',
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-illegal-entity-selector',
'param-illegal'
);
}
diff --git a/repo/includes/Api/ParseValue.php b/repo/includes/Api/ParseValue.php
index c5bcb6d..d7b6a86 100644
--- a/repo/includes/Api/ParseValue.php
+++ b/repo/includes/Api/ParseValue.php
@@ -130,14 +130,20 @@
if ( empty( $name ) ) {
// If neither 'datatype' not 'parser' is given, tell
the client to use 'datatype'.
- $this->errorReporter->dieError( 'No datatype given',
'param-illegal' );
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-not-recognized-datatype',
+ 'param-illegal'
+ );
}
try {
$parser = $this->valueParserFactory->newParser( $name,
$options );
return $parser;
} catch ( OutOfBoundsException $ex ) {
- $this->errorReporter->dieError( "Unknown datatype
`$name`", 'unknown-datatype' );
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-not-recognized-datatype',
+ 'unknown-datatype'
+ );
throw new LogicException( 'dieError() did not throw an
exception' );
}
}
@@ -152,7 +158,10 @@
if ( empty( $name ) ) {
// 'datatype' parameter is required for validation.
- $this->errorReporter->dieError( 'No datatype given',
'param-illegal' );
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-not-recognized-datatype',
+ 'param-illegal'
+ );
}
// Note: For unknown datatype, we'll get an empty list.
diff --git a/repo/includes/Api/SetQualifier.php
b/repo/includes/Api/SetQualifier.php
index cb7905b..9535aa5 100644
--- a/repo/includes/Api/SetQualifier.php
+++ b/repo/includes/Api/SetQualifier.php
@@ -168,8 +168,8 @@
$propertyId = $this->modificationHelper->getEntityIdFromString(
$params['property'] );
if ( !( $propertyId instanceof PropertyId ) ) {
- $this->errorReporter->dieError(
- $propertyId->getSerialization() . ' does not
appear to be a property ID',
+ $this->errorReporter->dieWithError(
+ 'wikibase-api-invalid-property-id',
'param-illegal'
);
}
diff --git a/repo/includes/ChangeOp/ChangeOpStatement.php
b/repo/includes/ChangeOp/ChangeOpStatement.php
index 784762e..cba6a20 100644
--- a/repo/includes/ChangeOp/ChangeOpStatement.php
+++ b/repo/includes/ChangeOp/ChangeOpStatement.php
@@ -10,6 +10,7 @@
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\Services\Statement\GuidGenerator;
use Wikibase\DataModel\Services\Statement\StatementGuidParser;
+use Wikibase\DataModel\Services\Statement\StatementGuidParsingException;
use Wikibase\DataModel\Services\Statement\StatementGuidValidator;
use Wikibase\DataModel\Statement\Statement;
use Wikibase\DataModel\Statement\StatementList;
@@ -128,7 +129,11 @@
* @throws ChangeOpException
*/
private function validateStatementGuid( EntityId $entityId ) {
- $guid = $this->guidParser->parse( $this->statement->getGuid() );
+ try {
+ $guid = $this->guidParser->parse(
$this->statement->getGuid() );
+ } catch ( StatementGuidParsingException $ex ) {
+ throw new ChangeOpException( 'Statement GUID can not be
parsed' );
+ }
if ( !$this->guidValidator->validate( $guid->getSerialization()
) ) {
throw new ChangeOpException( 'Statement does not have a
valid GUID' );
diff --git a/repo/tests/phpunit/includes/Api/EditEntityTest.php
b/repo/tests/phpunit/includes/Api/EditEntityTest.php
index 3aecc33..dac9a01 100644
--- a/repo/tests/phpunit/includes/Api/EditEntityTest.php
+++ b/repo/tests/phpunit/includes/Api/EditEntityTest.php
@@ -697,8 +697,9 @@
} ] }'
),
'e' => array( 'exception' => array(
- // FIXME: Does this also need fixing?
- 'type' =>
StatementGuidParsingException::class
+ 'type' => ApiUsageException::class,
+ 'code' => 'modification-failed',
+ 'message' => 'Statement GUID can not be
parsed',
) )
),
'removing valid claim with no guid fails' => array(
@@ -902,7 +903,7 @@
'e' => [ 'exception' => [
'type' => ApiUsageException::class,
'code' => 'not-recognized',
- 'message-key' =>
'wikibase-api-not-recognized',
+ 'message-key' =>
'wikibase-api-illegal-entity-remove',
] ],
],
);
diff --git a/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
b/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
index 0e25086..366023a 100644
--- a/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
+++ b/repo/tests/phpunit/includes/Api/EntitySavingHelperTest.php
@@ -185,16 +185,20 @@
public function provideLoadEntity_fail() {
return [
'no params' => [
- [], 'no-entity-id'
+ [],
+ 'no-entity-id'
],
'baserevid but no entity' => [
- [ 'baserevid' => 17 ], 'param-illegal'
+ [ 'baserevid' => 17 ],
+ 'param-illegal'
],
'new bad' => [
- [ 'new' => 'bad' ], 'no-such-entity-type'
+ [ 'new' => 'bad' ],
+ 'no-such-entity-type'
],
'unknown entity' => [
- [ 'entity' => 'Q123' ], 'no-such-entity'
+ [ 'entity' => 'Q123' ],
+ 'no-such-entity'
],
];
}
diff --git a/repo/tests/phpunit/includes/Api/IndependentWikibaseApiTestCase.php
b/repo/tests/phpunit/includes/Api/IndependentWikibaseApiTestCase.php
index 1aa9209..bd89154 100644
--- a/repo/tests/phpunit/includes/Api/IndependentWikibaseApiTestCase.php
+++ b/repo/tests/phpunit/includes/Api/IndependentWikibaseApiTestCase.php
@@ -61,23 +61,33 @@
/**
* Do the test for exceptions from Api queries.
* @param array $params array of params for the api query
- * @param array $exception details of the exception to expect
(type,code,message)
+ * @param array $exception Details of the exception to expect (type,
code, message, message-key).
*/
public function doTestQueryExceptions( $params, $exception ) {
try {
$this->doApiRequest( $params );
- $this->fail( 'Failed to throw ApiUsageException' );
+ $this->fail( 'Failed to throw ApiUsageException' );
} catch ( ApiUsageException $e ) {
if ( array_key_exists( 'type', $exception ) ) {
$this->assertInstanceOf( $exception['type'], $e
);
}
+
if ( array_key_exists( 'code', $exception ) ) {
$this->assertEquals( $exception['code'],
$e->getCodeString() );
}
+
if ( array_key_exists( 'message', $exception ) ) {
$this->assertContains( $exception['message'],
$e->getMessage() );
}
+
+ if ( array_key_exists( 'message-key', $exception ) ) {
+ $status = $e->getStatusValue();
+ $this->assertTrue(
+ $status->hasMessage(
$exception['message-key'] ),
+ 'Status message key'
+ );
+ }
}
}
diff --git a/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
b/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
index aa83ce8..2b6879b 100644
--- a/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
+++ b/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
@@ -3,7 +3,6 @@
namespace Wikibase\Repo\Tests\Api;
use ApiTestCase;
-use Exception;
use MediaWiki\MediaWikiServices;
use OutOfBoundsException;
use Revision;
@@ -167,7 +166,7 @@
* Do the test for exceptions from Api queries.
*
* @param array $params Array of params for the API query.
- * @param array $exception Details of the exception to expect (type,
code, message).
+ * @param array $exception Details of the exception to expect (type,
code, message, message-key).
*/
protected function doTestQueryExceptions( array $params, array
$exception ) {
try {
@@ -184,9 +183,11 @@
if ( array_key_exists( 'type', $exception ) ) {
$this->assertInstanceOf( $exception['type'], $e
);
}
+
if ( array_key_exists( 'code', $exception ) ) {
$this->assertEquals( $exception['code'],
$e->getCodeString() );
}
+
if ( array_key_exists( 'message', $exception ) ) {
$this->assertContains( $exception['message'],
$e->getMessage() );
}
@@ -197,16 +198,6 @@
$status->hasMessage(
$exception['message-key'] ),
'Status message key'
);
- }
- } catch ( Exception $e ) {
- if ( $e instanceof
\PHPUnit_Framework_AssertionFailedError ) {
- throw $e;
- }
- if ( array_key_exists( 'type', $exception ) ) {
- $this->assertInstanceOf( $exception['type'], $e
);
- }
- if ( array_key_exists( 'message', $exception ) ) {
- $this->assertContains( $exception['message'],
$e->getMessage() );
}
}
}
diff --git a/repo/tests/phpunit/includes/ChangeOp/ChangeOpStatementTest.php
b/repo/tests/phpunit/includes/ChangeOp/ChangeOpStatementTest.php
index b8deaea..1a3be10 100644
--- a/repo/tests/phpunit/includes/ChangeOp/ChangeOpStatementTest.php
+++ b/repo/tests/phpunit/includes/ChangeOp/ChangeOpStatementTest.php
@@ -248,9 +248,14 @@
public function applyInvalidThrowsExceptionProvider() {
$itemEmpty = new Item( new ItemId( 'Q888' ) );
+ $noValueSnak = new PropertyNoValueSnak( new PropertyId( 'P45' )
);
+ $someValueSnak = new PropertySomeValueSnak( new PropertyId(
'P44' ) );
- $item777 = $this->makeNewItemWithStatement( 'Q777', new
PropertyNoValueSnak( 45 ) );
- $item666 = $this->makeNewItemWithStatement( 'Q666', new
PropertySomeValueSnak( 44 ) );
+ $statementWithInvalidGuid = new Statement( $noValueSnak );
+ $statementWithInvalidGuid->setGuid( 'Q0$' );
+
+ $item777 = $this->makeNewItemWithStatement( 'Q777',
$noValueSnak );
+ $item666 = $this->makeNewItemWithStatement( 'Q666',
$someValueSnak );
$item777Statements = $item777->getStatements()->toArray();
$item666Statements = $item666->getStatements()->toArray();
@@ -264,6 +269,7 @@
// test adding statements with guids from other items (these
shouldn't be added)
return array(
+ array( $itemEmpty, $statementWithInvalidGuid ),
array( $itemEmpty, $statements[666] ),
array( $itemEmpty, $statements[777] ),
array( $item666, $statements[777] ),
--
To view, visit https://gerrit.wikimedia.org/r/360871
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5c26fc968fd800a9df8e65f1cf7e099fd3644648
Gerrit-PatchSet: 6
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Jonas Kress (WMDE) <[email protected]>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: WMDE-leszek <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
