jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/353545 )
Change subject: Don't use deprecated methods in ApiErrorReporter.
......................................................................
Don't use deprecated methods in ApiErrorReporter.
Bug: T153359
Change-Id: Ib5b1eae2ab66e633a438d2705d7b96f386f16939
---
M repo/includes/Api/ApiErrorReporter.php
M repo/tests/phpunit/includes/Api/ApiErrorReporterTest.php
M repo/tests/phpunit/includes/Api/EditEntityTest.php
M repo/tests/phpunit/includes/Api/ModifyTermTestCase.php
M repo/tests/phpunit/includes/Api/SetAliasesTest.php
M repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
6 files changed, 133 insertions(+), 76 deletions(-)
Approvals:
WMDE-leszek: Looks good to me, but someone else must approve
Legoktm: Looks good to me, approved
jenkins-bot: Verified
Anomie: Looks good to me, but someone else must approve
diff --git a/repo/includes/Api/ApiErrorReporter.php
b/repo/includes/Api/ApiErrorReporter.php
index 6249bdd..b7276dc 100644
--- a/repo/includes/Api/ApiErrorReporter.php
+++ b/repo/includes/Api/ApiErrorReporter.php
@@ -3,6 +3,10 @@
namespace Wikibase\Repo\Api;
use ApiBase;
+use ApiErrorFormatter;
+use ApiErrorFormatter_BackCompat;
+use ApiMessage;
+use ApiRawMessage;
use ApiResult;
use Exception;
use InvalidArgumentException;
@@ -10,8 +14,9 @@
use LogicException;
use MediaWiki\MediaWikiServices;
use Message;
-use Status;
use ApiUsageException;
+use MessageSpecifier;
+use StatusValue;
use Wikibase\Repo\Localizer\ExceptionLocalizer;
/**
@@ -54,13 +59,20 @@
}
/**
- * Reports any warnings in the Status object on the warnings section
+ * Reports any warnings in the StatusValue object on the warnings
section
* of the result.
*
- * @param Status $status
+ * @param StatusValue $status
*/
- public function reportStatusWarnings( Status $status ) {
- $warnings = $status->getWarningsArray();
+ public function reportStatusWarnings( StatusValue $status ) {
+ $formatter = $this->apiModule->getErrorFormatter();
+
+ if ( !( $formatter instanceof ApiErrorFormatter_BackCompat ) ) {
+ $formatter->addMessagesFromStatus(
$this->apiModule->getModulePath(), $status, 'warning' );
+ return;
+ }
+
+ $warnings = $status->getErrorsByType( 'warning' );
if ( !empty( $warnings ) ) {
$warnings = $this->convertMessagesToResult( $warnings );
@@ -70,6 +82,8 @@
/**
* Set warning section for this module.
+ *
+ * @note Only needed when ApiErrorFormatter_BackCompat is in use.
*
* @param string $key
* @param string|array $warningData Warning message
@@ -87,7 +101,7 @@
}
/**
- * Aborts the request with an error based on the given (fatal) Status.
+ * Aborts the request with an error based on the given (fatal)
StatusValue.
* This is intended as an alternative for ApiBase::dieUsage().
*
* If possible, a localized error message based on the exception is
@@ -98,28 +112,50 @@
*
* @see ApiBase::dieUsage()
*
- * @param Status $status The status to report. $status->getMessage()
will be used
+ * @param StatusValue $status The status to report.
$status->getMessage() will be used
* to generate the error's free form description.
* @param string $errorCode A code identifying the error.
- * @param int $httpRespCode The HTTP error code to send to the client
+ * @param int $httpRespCode Currently ignored!
* @param array|null $extradata Any extra data to include in the error
report
*
* @throws ApiUsageException
* @throws LogicException
*/
- public function dieStatus( Status $status, $errorCode, $httpRespCode =
0, $extradata = array() ) {
+ public function dieStatus( StatusValue $status, $errorCode,
$httpRespCode = 0, $extradata = array() ) {
if ( $status->isOK() ) {
- throw new InvalidArgumentException( 'called dieStatus()
with a non-fatal Status!' );
+ throw new InvalidArgumentException( 'called dieStatus()
with a non-fatal StatusValue!' );
}
+ $error = $this->getPlainErrorMessageFromStatus( $status );
+ $msg = $this->getMessageForCode( $errorCode, $error, $extradata
);
+
+ $extendedStatus = StatusValue::newFatal( $msg );
+ $extendedStatus->merge( $status, true );
+ $status = $extendedStatus;
+
$this->addStatusToResult( $status, $extradata );
+ $msg->setApiData( $extradata );
- //XXX: when to prefer $statusCode over $errorCode?
- list( , $description ) = $this->apiModule->getErrorFromStatus(
$status );
+ $stats =
MediaWikiServices::getInstance()->getStatsdDataFactory();
+ $stats->increment( 'wikibase.repo.api.errors.total' );
- $this->throwUsageException( $description, $errorCode,
$httpRespCode, $extradata );
+ $this->apiModule->dieStatus( $status );
+ }
- throw new LogicException( 'ApiUsageException not thrown' );
+ /**
+ * @param StatusValue $status
+ *
+ * @return string|null a plain text english error message, or null
+ */
+ private function getPlainErrorMessageFromStatus( StatusValue $status ) {
+ $errors = $status->getErrorsByType( 'error' );
+ if ( !$errors ) {
+ return null;
+ }
+ $msg = ApiMessage::create( $errors[0] )
+ ->inLanguage( 'en' )
+ ->useDatabase( false );
+ return ApiErrorFormatter::stripMarkup( $msg->text() );
}
/**
@@ -170,15 +206,10 @@
* @throws LogicException
*/
public function dieMessage( $errorCode /*...*/ ) {
- $messageName = "wikibase-api-$errorCode";
+ $messageKey = "wikibase-api-$errorCode";
$params = func_get_args();
array_shift( $params );
- $message = wfMessage( $messageName, $params );
-
- if ( !$message->exists() ) {
- // TODO: log warning
- // TODO: replace with generic message
- }
+ $message = wfMessage( $messageKey, $params );
$this->dieMessageObject( $message, $errorCode );
@@ -203,11 +234,9 @@
* @throws LogicException
*/
private function dieMessageObject( Message $message, $errorCode,
$httpRespCode = 0, $extradata = array() ) {
- $description = $message->inLanguage( 'en' )->useDatabase( false
)->plain();
-
$this->addMessageToResult( $message, $extradata );
- $this->throwUsageException( $description, $errorCode,
$httpRespCode, $extradata );
+ $this->throwUsageException( $message, $errorCode, $extradata,
$httpRespCode );
throw new LogicException( 'ApiUsageException not thrown' );
}
@@ -232,45 +261,60 @@
* @throws LogicException
*/
public function dieError( $description, $errorCode, $httpRespCode = 0,
$extradata = array() ) {
- //TODO: try a reverse lookup in ApiBase::$messageMap
- $messageKey = "wikibase-api-$errorCode";
- $message = wfMessage( $messageKey );
+ $message = $this->getMessageForCode( $errorCode, $description,
$extradata );
- if ( $message->exists() ) {
- $this->addMessageToResult( $message, $extradata );
+ $this->addMessageToResult( $message, $extradata );
+ $message->setApiData( $extradata );
- $text = $message->inLanguage( 'en' )->useDatabase(
false )->plain();
-
- if ( $description == '' ) {
- $description = $text;
- } else {
- $description = "$text ($description)";
- }
- }
-
- $this->throwUsageException( $description, $errorCode,
$httpRespCode, $extradata );
+ $this->throwUsageException( $message, $errorCode, $extradata,
$httpRespCode );
throw new LogicException( 'ApiUsageException not thrown' );
}
/**
+ * @param string $errorCode
+ * @param string|null $description Plain text english message
+ * @param array|null $data
+ *
+ * @return ApiMessage
+ */
+ private function getMessageForCode( $errorCode, $description = null,
array $data = null ) {
+ $messageKey = "wikibase-api-$errorCode";
+
+ $message = wfMessage( $messageKey );
+
+ if ( !$message->exists() ) {
+ // NOTE: Use key fallback, so the nominal message key
will be the original.
+ $params = $description === null ? [] : [ $description ];
+ $message = wfMessage( [ $messageKey, 'rawmessage' ],
$params );
+ }
+
+ return ApiMessage::create( $message, $errorCode, $data );
+ }
+
+ /**
* Throws a ApiUsageException by calling ApiBase::dieUsage().
*
- * @see ApiBase::dieUsage()
+ * @see ApiBase::dieWithError()
*
- * @param string $description
- * @param string $errorCode
- * @param int $httpRespCode
- * @param null|array $extradata
+ * @param string|array|MessageSpecifier $msg A plain text (English)
message, or a message key
+ * and parameters as an array, or a MessageSpecifier object.
+ * @param string|null $code See ApiMessage::create()
+ * @param array|null $data See ApiMessage::create()
+ * @param int $httpCode HTTP error code to use
*
* @throws ApiUsageException
* @throws LogicException
*/
- private function throwUsageException( $description, $errorCode,
$httpRespCode = 0, $extradata = null ) {
+ private function throwUsageException( $msg, $code, array $data = null,
$httpCode = 0 ) {
$stats =
MediaWikiServices::getInstance()->getStatsdDataFactory();
$stats->increment( 'wikibase.repo.api.errors.total' );
- $this->apiModule->getMain()->dieUsage( $description,
$errorCode, $httpRespCode, $extradata );
+ if ( is_string( $msg ) ) {
+ $msg = new ApiRawMessage( $msg, $code, $data );
+ }
+
+ $this->apiModule->getMain()->dieWithError( $msg, $code, $data,
$httpCode );
throw new LogicException( 'ApiUsageException not thrown' );
}
@@ -302,21 +346,30 @@
}
/**
- * Add the messages from the given Status object to the $data array,
+ * Add the messages from the given StatusValue object to the $data
array,
* for use in an error report.
*
- * @param Status $status
+ * @param StatusValue $status
* @param array|null &$data
*
* @throws InvalidArgumentException
*/
- public function addStatusToResult( Status $status, &$data ) {
- $messageSpecs = $status->getErrorsArray();
+ public function addStatusToResult( StatusValue $status, &$data ) {
+ // Use Wikibase specific representation of messages in the
result.
+ // TODO: This should be phased out in favor of using
ApiErrorFormatter, see below.
+ $messageSpecs = $status->getErrorsByType( 'error' );
$messages = $this->convertToMessageList( $messageSpecs );
foreach ( $messages as $message ) {
$this->addMessageToResult( $message, $data );
}
+
+ // Additionally, provide new (2016) API error reporting output.
+ $this->apiModule->getErrorFormatter()->addMessagesFromStatus(
+ $this->apiModule->getModulePath(),
+ $status,
+ 'error'
+ );
}
/**
@@ -338,10 +391,10 @@
* In that case, the 'params' field is ignored and the parameter list
is taken from the
* Message object.
*
- * This provides support for message lists coming from
Status::getErrorsByType() as well as
+ * This provides support for message lists coming from
StatusValue::getErrorsByType() as well as
* Title::getUserPermissionsErrors() etc.
*
- * @param array $messageSpecs a list of errors, as returned by
Status::getErrorsByType()
+ * @param array $messageSpecs a list of errors, as returned by
StatusValue::getErrorsByType()
* or Title::getUserPermissionsErrors()
*
* @return array a result structure containing the messages from
$errors as well as what
@@ -384,7 +437,7 @@
*
* @see convertToMessage()
*
- * @param array $messageSpecs a list of errors, as returned by
Status::getErrorsByType()
+ * @param array $messageSpecs a list of errors, as returned by
StatusValue::getErrorsByType()
* or Title::getUserPermissionsErrors().
*
* @return array a result structure containing the messages from
$errors as well as what
@@ -436,7 +489,7 @@
* Utility function for converting a message specified as a string or
array
* to a Message object. Returns null if this is not possible.
*
- * The formats supported by this method are the formats used by the
Status class as well as
+ * The formats supported by this method are the formats used by the
StatusValue class as well as
* the one used by Title::getUserPermissionsErrors().
*
* The spec may be structured as follows:
diff --git a/repo/tests/phpunit/includes/Api/ApiErrorReporterTest.php
b/repo/tests/phpunit/includes/Api/ApiErrorReporterTest.php
index 3b0ea42..e0ccd61 100644
--- a/repo/tests/phpunit/includes/Api/ApiErrorReporterTest.php
+++ b/repo/tests/phpunit/includes/Api/ApiErrorReporterTest.php
@@ -33,21 +33,22 @@
array $expectedDataFields,
ApiUsageException $ex
) {
+ // Using deprecated getMessageArray() because
ApiUsageException::getApiMessage() isn't public.
$messageArray = $ex->getMessageArray();
$this->assertArrayHasKey( 'code', $messageArray );
$this->assertArrayHasKey( 'info', $messageArray );
if ( $info !== null ) {
- $this->assertRegExp( $info, $messageArray['info'] );
+ $this->assertRegExp( $info, $messageArray['info'],
'error info message' );
}
if ( $code !== null ) {
- $this->assertEquals( $code, $messageArray['code'] );
+ $this->assertSame( $code, $messageArray['code'], 'error
code' );
}
if ( $httpStatusCode ) {
- $this->assertEquals( $httpStatusCode, $ex->getCode() );
+ $this->assertSame( $httpStatusCode, $ex->getCode(),
'HTTP status code' );
}
foreach ( $expectedDataFields as $path => $value ) {
@@ -68,7 +69,7 @@
$this->assertInternalType( 'string', $data, $name );
$this->assertRegExp( $expected, $data, $name );
} else {
- $this->assertEquals( $expected, $data, $name );
+ $this->assertSame( $expected, $data, $name );
}
}
@@ -109,7 +110,7 @@
'$code' => 'no-such-sitelink',
'$httpStatusCode' => 0,
'$extradata' => array( 'fruit' => 'Banana' ),
- '$infoPattern' => '/sitelink.*\(ugh!\)$/',
+ '$infoPattern' => '/Could not find a sitelink/',
'$expectedData' => array(
'fruit' => 'Banana',
'messages/0/name' =>
'wikibase-api-no-such-sitelink',
@@ -207,24 +208,22 @@
'known error code' => array(
'$status' => $status,
'$code' => 'errorreporter-test-ugh',
- '$httpStatusCode' => 0,
'$extradata' => null,
'$infoPattern' => '/sitelink/',
'$expectedData' => array(
- 'messages/0/name' =>
'wikibase-api-no-such-sitelink',
- 'messages/0/html' => '/gefunden/', //
in German
- 'messages/1/name' =>
'wikibase-noentity',
- 'messages/1/parameters/0' => 'Q123',
- 'messages/1/html' => '/ist nicht
vorhanden/', // in German
+ 'messages/0/name' =>
'wikibase-api-errorreporter-test-ugh',
+ 'messages/1/name' =>
'wikibase-api-no-such-sitelink',
+ 'messages/1/html' => '/gefunden/', //
in German
+ 'messages/2/name' =>
'wikibase-noentity',
+ 'messages/2/parameters/0' => 'Q123',
+ 'messages/2/html' => '/ist nicht
vorhanden/', // in German
),
),
// Any extra data should be passed through.
- // The HTTP status code should be used.
'extradata' => array(
'$status' => $status,
'$code' => 'errorreporter-test-ugh',
- '$httpStatusCode' => 555,
'$extradata' => array( 'fruit' => 'Banana' ),
'$infoPattern' => null,
'$expectedData' => array(
@@ -240,7 +239,6 @@
public function testDieStatus(
Status $status,
$code,
- $httpStatusCode,
array $extradata = null,
$infoPattern,
array $expectedDataFields
@@ -250,10 +248,10 @@
$reporter = new ApiErrorReporter( $api, $localizer,
Language::factory( 'de' ) );
try {
- $reporter->dieStatus( $status, $code, $httpStatusCode,
$extradata );
+ $reporter->dieStatus( $status, $code, 0, $extradata );
$this->fail( 'ApiUsageException was not thrown!' );
} catch ( ApiUsageException $ex ) {
- $this->assertUsageException( $infoPattern, $code,
$httpStatusCode, $expectedDataFields, $ex );
+ $this->assertUsageException( $infoPattern, $code, 0,
$expectedDataFields, $ex );
}
}
diff --git a/repo/tests/phpunit/includes/Api/EditEntityTest.php
b/repo/tests/phpunit/includes/Api/EditEntityTest.php
index ef7d4b4..3aecc33 100644
--- a/repo/tests/phpunit/includes/Api/EditEntityTest.php
+++ b/repo/tests/phpunit/includes/Api/EditEntityTest.php
@@ -902,7 +902,7 @@
'e' => [ 'exception' => [
'type' => ApiUsageException::class,
'code' => 'not-recognized',
- 'message' => '"remove" should not be a
top-level key'
+ 'message-key' =>
'wikibase-api-not-recognized',
] ],
],
);
diff --git a/repo/tests/phpunit/includes/Api/ModifyTermTestCase.php
b/repo/tests/phpunit/includes/Api/ModifyTermTestCase.php
index bd82a75..4023870 100644
--- a/repo/tests/phpunit/includes/Api/ModifyTermTestCase.php
+++ b/repo/tests/phpunit/includes/Api/ModifyTermTestCase.php
@@ -188,7 +188,6 @@
'e' => array( 'exception' => array(
'type' => ApiUsageException::class,
'code' => 'no-such-entity-link',
- 'message' => 'No entity found matching
site link'
) )
),
array( //7
@@ -196,7 +195,6 @@
'e' => array( 'exception' => array(
'type' => ApiUsageException::class,
'code' => 'param-illegal',
- 'message' => 'Either provide the item
"id" or pairs'
) )
),
array( //8
@@ -204,7 +202,6 @@
'e' => array( 'exception' => array(
'type' => ApiUsageException::class,
'code' => 'param-illegal',
- 'message' => 'Either provide the item
"id" or pairs'
) )
),
);
diff --git a/repo/tests/phpunit/includes/Api/SetAliasesTest.php
b/repo/tests/phpunit/includes/Api/SetAliasesTest.php
index 7efae37..06707f2 100644
--- a/repo/tests/phpunit/includes/Api/SetAliasesTest.php
+++ b/repo/tests/phpunit/includes/Api/SetAliasesTest.php
@@ -211,7 +211,6 @@
'e' => array( 'exception' => array(
'type' => ApiUsageException::class,
'code' => 'no-such-entity-link',
- 'message' => 'No entity found matching
site link'
) )
),
array( //7
@@ -219,7 +218,6 @@
'e' => array( 'exception' => array(
'type' => ApiUsageException::class,
'code' => 'param-illegal',
- 'message' => 'Either provide the item
"id" or pairs'
) )
),
array( //8
diff --git a/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
b/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
index b6dfd5c..aa83ce8 100644
--- a/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
+++ b/repo/tests/phpunit/includes/Api/WikibaseApiTestCase.php
@@ -190,7 +190,18 @@
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'
+ );
+ }
} catch ( Exception $e ) {
+ if ( $e instanceof
\PHPUnit_Framework_AssertionFailedError ) {
+ throw $e;
+ }
if ( array_key_exists( 'type', $exception ) ) {
$this->assertInstanceOf( $exception['type'], $e
);
}
--
To view, visit https://gerrit.wikimedia.org/r/353545
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib5b1eae2ab66e633a438d2705d7b96f386f16939
Gerrit-PatchSet: 14
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Aleksey Bekh-Ivanov (WMDE) <[email protected]>
Gerrit-Reviewer: Anomie <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Gergő Tisza <[email protected]>
Gerrit-Reviewer: Legoktm <[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
