From a90bbf1a484c9d22cb04770e92ce18fe3e4bf398 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 8 Dec 2016 13:38:45 -0500 Subject: [PATCH] Add ILocalizedException interface We already throw around some exceptions that are localized (ErrorPageError and its subclasses, MalformedTitleException), but there's no standard way to recognize them. Let's change that. Then let's use them in the API to be able to have internationalized errors when such exceptions are caught, instead of wrapping the English-language version. Change-Id: Iac7c90f92a889f8de9dae373547c07b884addaea --- RELEASE-NOTES-1.29 | 4 + autoload.php | 2 + includes/api/ApiBase.php | 14 ++++ includes/api/ApiEditPage.php | 8 +- includes/api/ApiErrorFormatter.php | 73 +++++++++++++++++++ includes/api/ApiImport.php | 2 +- includes/api/ApiMain.php | 4 +- includes/api/ApiPageSet.php | 2 +- includes/api/ApiParse.php | 8 +- includes/api/ApiQueryStashImageInfo.php | 5 +- includes/api/ApiUpload.php | 34 ++++++--- includes/api/ApiUsageException.php | 9 ++- includes/api/i18n/en.json | 2 + includes/api/i18n/qqq.json | 2 + includes/exception/ErrorPageError.php | 20 +++-- includes/exception/LocalizedException.php | 66 +++++++++++++++++ includes/exception/PermissionsError.php | 3 + .../libs/rdbms/exception/DBExpectedError.php | 10 ++- includes/title/MalformedTitleException.php | 10 ++- 19 files changed, 241 insertions(+), 37 deletions(-) create mode 100644 includes/exception/LocalizedException.php diff --git a/RELEASE-NOTES-1.29 b/RELEASE-NOTES-1.29 index 986ecd891b..e51dacf31b 100644 --- a/RELEASE-NOTES-1.29 +++ b/RELEASE-NOTES-1.29 @@ -26,6 +26,8 @@ production. === New features in 1.29 === * (T5233) A cookie can now be set when a user is autoblocked, to track that user if they move to a new IP address. This is disabled by default. +* Added ILocalizedException interface to standardize the use of localized + exceptions, largely so the API can handle them more sensibly. === External library changes in 1.29 === @@ -49,6 +51,8 @@ production. using the new 'errorformat', 'errorlang', and 'errorsuselocal' parameters. * API error codes may have changed. Most notably, errors from modules using parameter prefixes (e.g. all query submodules) will no longer be prefixed. +* ApiPageSet-using modules will report the 'invalidreason' using the specified + 'errorformat'. * action=emailuser may return a "Warnings" status, and now returns 'warnings' and 'errors' subelements (as applicable) instead of 'message'. * action=imagerotate returns an 'errors' subelement rather than 'errormessage'. diff --git a/autoload.php b/autoload.php index e079686bc4..60ce8f64ea 100644 --- a/autoload.php +++ b/autoload.php @@ -598,6 +598,7 @@ $wgAutoloadLocalClasses = [ 'ILBFactory' => __DIR__ . '/includes/libs/rdbms/lbfactory/ILBFactory.php', 'ILoadBalancer' => __DIR__ . '/includes/libs/rdbms/loadbalancer/ILoadBalancer.php', 'ILoadMonitor' => __DIR__ . '/includes/libs/rdbms/loadmonitor/ILoadMonitor.php', + 'ILocalizedException' => __DIR__ . '/includes/exception/LocalizedException.php', 'IMaintainableDatabase' => __DIR__ . '/includes/libs/rdbms/database/IMaintainableDatabase.php', 'IP' => __DIR__ . '/includes/libs/IP.php', 'IPSet' => __DIR__ . '/includes/compat/IPSetCompat.php', @@ -759,6 +760,7 @@ $wgAutoloadLocalClasses = [ 'LocalSettingsGenerator' => __DIR__ . '/includes/installer/LocalSettingsGenerator.php', 'LocalisationCache' => __DIR__ . '/includes/cache/localisation/LocalisationCache.php', 'LocalisationCacheBulkLoad' => __DIR__ . '/includes/cache/localisation/LocalisationCacheBulkLoad.php', + 'LocalizedException' => __DIR__ . '/includes/exception/LocalizedException.php', 'LockManager' => __DIR__ . '/includes/libs/lockmanager/LockManager.php', 'LockManagerGroup' => __DIR__ . '/includes/filebackend/lockmanager/LockManagerGroup.php', 'LogEntry' => __DIR__ . '/includes/logging/LogEntry.php', diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index a40593f6bd..65fcb99d23 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -1723,6 +1723,20 @@ abstract class ApiBase extends ContextSource { throw ApiUsageException::newWithMessage( $this, $msg, $code, $data, $httpCode ); } + /** + * Abort execution with an error derived from an exception + * + * @since 1.29 + * @param Exception|Throwable $exception See ApiErrorFormatter::getMessageFromException() + * @param array $options See ApiErrorFormatter::getMessageFromException() + * @throws ApiUsageException always + */ + public function dieWithException( $exception, array $options = [] ) { + $this->dieWithError( + $this->getErrorFormatter()->getMessageFromException( $exception, $options ) + ); + } + /** * Adds a warning to the output, else dies * diff --git a/includes/api/ApiEditPage.php b/includes/api/ApiEditPage.php index 6b568701fd..e5c73b3cbf 100644 --- a/includes/api/ApiEditPage.php +++ b/includes/api/ApiEditPage.php @@ -140,11 +140,9 @@ class ApiEditPage extends ApiBase { try { $content = ContentHandler::makeContent( $text, $this->getTitle() ); } catch ( MWContentSerializationException $ex ) { - // @todo: Internationalize MWContentSerializationException - $this->dieWithError( - [ 'apierror-contentserializationexception', wfEscapeWikiText( $ex->getMessage() ) ], - 'parseerror' - ); + $this->dieWithException( $ex, [ + 'wrap' => ApiMessage::create( 'apierror-contentserializationexception', 'parseerror' ) + ] ); return; } } else { diff --git a/includes/api/ApiErrorFormatter.php b/includes/api/ApiErrorFormatter.php index 4fb19b88d5..f246203a3a 100644 --- a/includes/api/ApiErrorFormatter.php +++ b/includes/api/ApiErrorFormatter.php @@ -142,6 +142,66 @@ class ApiErrorFormatter { } } + /** + * Get an ApiMessage from an exception + * @since 1.29 + * @param Exception|Throwable $exception + * @param array $options + * - wrap: (string|array|MessageSpecifier) Used to wrap the exception's + * message. The exception's message will be added as the final parameter. + * - code: (string) Default code + * - data: (array) Extra data + * @return ApiMessage + */ + public function getMessageFromException( $exception, array $options = [] ) { + $options += [ 'code' => null, 'data' => [] ]; + + if ( $exception instanceof ILocalizedException ) { + $msg = $exception->getMessageObject(); + $params = []; + } else { + // Extract code and data from the exception, if applicable + if ( $exception instanceof UsageException ) { + $data = $exception->getMessageArray(); + if ( !isset( $options['code'] ) ) { + $options['code'] = $data['code']; + } + unset( $data['code'], $data['info'] ); + $options['data'] = array_merge( $data['code'], $options['data'] ); + } + + if ( isset( $options['wrap'] ) ) { + $msg = $options['wrap']; + } else { + $msg = new RawMessage( '$1' ); + if ( !isset( $options['code'] ) ) { + $options['code'] = 'internal_api_error_' . get_class( $exception ); + } + } + $params = [ wfEscapeWikiText( $exception->getMessage() ) ]; + } + return ApiMessage::create( $msg, $options['code'], $options['data'] ) + ->params( $params ) + ->inLanguage( $this->lang ) + ->title( $this->getDummyTitle() ) + ->useDatabase( $this->useDB ); + } + + /** + * Format an exception as an array + * @since 1.29 + * @param Exception|Throwable $exception + * @param array $options See self::getMessageFromException(), plus + * - format: (string) Format override + * @return array + */ + public function formatException( $exception, array $options = [] ) { + return $this->formatMessage( + $this->getMessageFromException( $exception, $options ), + isset( $options['format'] ) ? $options['format'] : null + ); + } + /** * Format a message as an array * @param Message|array|string $msg Message. See ApiMessage::create(). @@ -335,6 +395,19 @@ class ApiErrorFormatter_BackCompat extends ApiErrorFormatter { ] + $msg->getApiData(); } + /** + * Format an exception as an array + * @since 1.29 + * @param Exception|Throwable $exception + * @param array $options See parent::formatException(), plus + * - bc: (bool) Return only the string, not an array + * @return array|string + */ + public function formatException( $exception, array $options = [] ) { + $ret = parent::formatException( $exception, $options ); + return empty( $options['bc'] ) ? $ret : $ret['info']; + } + protected function addWarningOrError( $tag, $modulePath, $msg ) { $value = self::stripMarkup( $msg->text() ); diff --git a/includes/api/ApiImport.php b/includes/api/ApiImport.php index 3f48f38a0e..dffd6b253f 100644 --- a/includes/api/ApiImport.php +++ b/includes/api/ApiImport.php @@ -83,7 +83,7 @@ class ApiImport extends ApiBase { try { $importer->doImport(); } catch ( Exception $e ) { - $this->dieWithError( [ 'apierror-import-unknownerror', wfEscapeWikiText( $e->getMessage() ) ] ); + $this->dieWithException( $e, [ 'wrap' => 'apierror-import-unknownerror' ] ); } $resultData = $reporter->getData(); diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index fe6ed417ca..54679a80a4 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -1046,7 +1046,9 @@ class ApiMain extends ApiBase { $params = [ 'apierror-exceptioncaught', WebRequest::getRequestId(), - wfEscapeWikiText( $e->getMessage() ) + $e instanceof ILocalizedException + ? $e->getMessageObject() + : wfEscapeWikiText( $e->getMessage() ) ]; } $messages[] = ApiMessage::create( $params, $code ); diff --git a/includes/api/ApiPageSet.php b/includes/api/ApiPageSet.php index 4cf896f1dc..160ce87365 100644 --- a/includes/api/ApiPageSet.php +++ b/includes/api/ApiPageSet.php @@ -1151,7 +1151,7 @@ class ApiPageSet extends ApiBase { $this->mAllPages[0][$title] = $this->mFakePageId; $this->mInvalidTitles[$this->mFakePageId] = [ 'title' => $title, - 'invalidreason' => $ex->getMessage(), + 'invalidreason' => $this->getErrorFormatter()->formatException( $ex, [ 'bc' => true ] ), ]; $this->mFakePageId--; continue; // There's nothing else we can do diff --git a/includes/api/ApiParse.php b/includes/api/ApiParse.php index 2263b8f83c..287ffb7c0e 100644 --- a/includes/api/ApiParse.php +++ b/includes/api/ApiParse.php @@ -225,11 +225,9 @@ class ApiParse extends ApiBase { try { $this->content = ContentHandler::makeContent( $text, $titleObj, $model, $format ); } catch ( MWContentSerializationException $ex ) { - // @todo: Internationalize MWContentSerializationException - $this->dieWithError( - [ 'apierror-contentserializationexception', wfEscapeWikiText( $ex->getMessage() ) ], - 'parseerror' - ); + $this->dieWithException( $ex, [ + 'wrap' => ApiMessage::create( 'apierror-contentserializationexception', 'parseerror' ) + ] ); } if ( $this->section !== false ) { diff --git a/includes/api/ApiQueryStashImageInfo.php b/includes/api/ApiQueryStashImageInfo.php index 981cb09483..abb827fe4f 100644 --- a/includes/api/ApiQueryStashImageInfo.php +++ b/includes/api/ApiQueryStashImageInfo.php @@ -63,11 +63,10 @@ class ApiQueryStashImageInfo extends ApiQueryImageInfo { $result->addIndexedTagName( [ 'query', $this->getModuleName() ], $modulePrefix ); } // @todo Update exception handling here to understand current getFile exceptions - // @todo Internationalize the exceptions } catch ( UploadStashFileNotFoundException $e ) { - $this->dieWithError( [ 'apierror-stashedfilenotfound', wfEscapeWikiText( $e->getMessage() ) ] ); + $this->dieWithException( $e, [ 'wrap' => 'apierror-stashedfilenotfound' ] ); } catch ( UploadStashBadPathException $e ) { - $this->dieWithError( [ 'apierror-stashpathinvalid', wfEscapeWikiText( $e->getMessage() ) ] ); + $this->dieWithException( $e, [ 'wrap' => 'apierror-stashpathinvalid' ] ); } } diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php index 6bdd68f937..311fa54a9f 100644 --- a/includes/api/ApiUpload.php +++ b/includes/api/ApiUpload.php @@ -320,12 +320,14 @@ class ApiUpload extends ApiBase { if ( $status->isGood() && !$status->getValue() ) { // Not actually a 'good' status... - $status->fatal( new ApiRawMessage( 'Invalid stashed file', 'stashfailed' ) ); + $status->fatal( new ApiMessage( 'apierror-stashinvalidfile', 'stashfailed' ) ); } } catch ( Exception $e ) { $debugMessage = 'Stashing temporary file failed: ' . get_class( $e ) . ' ' . $e->getMessage(); wfDebug( __METHOD__ . ' ' . $debugMessage . "\n" ); - $status = Status::newFatal( new ApiRawMessage( $e->getMessage(), 'stashfailed' ) ); + $status = Status::newFatal( $this->getErrorFormatter()->getMessageFromException( + $e, [ 'wrap' => new ApiMessage( 'apierror-stashexception', 'stashfailed' ) ] + ) ); } if ( $status->isGood() ) { @@ -564,7 +566,6 @@ class ApiUpload extends ApiBase { * @param array $verification */ protected function checkVerification( array $verification ) { - // @todo Move them to ApiBase's message map switch ( $verification['status'] ) { // Recoverable errors case UploadBase::MIN_LENGTH_PARTNAME: @@ -713,32 +714,41 @@ class ApiUpload extends ApiBase { /** * Handles a stash exception, giving a useful error to the user. - * @todo Internationalize the exceptions + * @todo Internationalize the exceptions then get rid of this * @param Exception $e * @return StatusValue */ protected function handleStashException( $e ) { - $err = wfEscapeWikiText( $e->getMessage() ); switch ( get_class( $exception ) ) { case 'UploadStashFileNotFoundException': - return StatusValue::newFatal( 'apierror-stashedfilenotfound', $err ); + $wrap = 'apierror-stashedfilenotfound'; + break; case 'UploadStashBadPathException': - return StatusValue::newFatal( 'apierror-stashpathinvalid', $err ); + $wrap = 'apierror-stashpathinvalid'; + break; case 'UploadStashFileException': - return StatusValue::newFatal( 'apierror-stashfilestorage', $err ); + $wrap = 'apierror-stashfilestorage'; + break; case 'UploadStashZeroLengthFileException': - return StatusValue::newFatal( 'apierror-stashzerolength', $err ); + $wrap = 'apierror-stashzerolength'; + break; case 'UploadStashNotLoggedInException': return StatusValue::newFatal( ApiMessage::create( [ 'apierror-mustbeloggedin', $this->msg( 'action-upload' ) ], 'stashnotloggedin' ) ); case 'UploadStashWrongOwnerException': - return StatusValue::newFatal( 'apierror-stashwrongowner', $err ); + $wrap = 'apierror-stashwrongowner'; + break; case 'UploadStashNoSuchKeyException': - return StatusValue::newFatal( 'apierror-stashnosuchfilekey', $err ); + $wrap = 'apierror-stashnosuchfilekey'; + break; default: - return StatusValue::newFatal( 'uploadstash-exception', get_class( $e ), $err ); + $wrap = [ 'uploadstash-exception', get_class( $e ) ]; + break; } + return StatusValue::newFatal( + $this->getErrorFormatter()->getMessageFromException( $e, [ 'wrap' => $wrap ] ) + ); } /** diff --git a/includes/api/ApiUsageException.php b/includes/api/ApiUsageException.php index 7e21ab5ba4..9dc1f92bbc 100644 --- a/includes/api/ApiUsageException.php +++ b/includes/api/ApiUsageException.php @@ -95,7 +95,7 @@ class UsageException extends MWException { * starts throwing ApiUsageException. Eventually UsageException will go away * and this will (probably) extend MWException directly. */ -class ApiUsageException extends UsageException { +class ApiUsageException extends UsageException implements ILocalizedException { protected $modulePath; protected $status; @@ -201,6 +201,13 @@ class ApiUsageException extends UsageException { ] + $enMsg->getApiData(); } + /** + * @inheritdoc + */ + public function getMessageObject() { + return $this->status->getMessage(); + } + /** * @return string */ diff --git a/includes/api/i18n/en.json b/includes/api/i18n/en.json index 02aa6dbc29..aa9e4acd47 100644 --- a/includes/api/i18n/en.json +++ b/includes/api/i18n/en.json @@ -1692,9 +1692,11 @@ "apierror-specialpage-cantexecute": "You don't have permission to view the results of this special page.", "apierror-stashedfilenotfound": "Could not find the file in the stash: $1.", "apierror-stashedit-missingtext": "No stashed text found with the given hash.", + "apierror-stashexception": "$1", "apierror-stashfailed-complete": "Chunked upload is already completed, check status for details.", "apierror-stashfailed-nosession": "No chunked upload session with this key.", "apierror-stashfilestorage": "Could not store upload in the stash: $1", + "apierror-stashinvalidfile": "Invalid stashed file.", "apierror-stashnosuchfilekey": "No such filekey: $1.", "apierror-stashpathinvalid": "File key of improper format or otherwise invalid: $1.", "apierror-stashwrongowner": "Wrong owner: $1", diff --git a/includes/api/i18n/qqq.json b/includes/api/i18n/qqq.json index 8cf338dddb..caf202c357 100644 --- a/includes/api/i18n/qqq.json +++ b/includes/api/i18n/qqq.json @@ -1587,8 +1587,10 @@ "apierror-stashedfilenotfound": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception text. Currently this is probably English, hopefully we'll fix that in the future.", "apierror-stashedit-missingtext": "{{doc-apierror}}", "apierror-stashfailed-complete": "{{doc-apierror}}", + "apierror-stashexception": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception text. May be English or localized, may or may not end in punctuation.", "apierror-stashfailed-nosession": "{{doc-apierror}}", "apierror-stashfilestorage": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception text, which may already end with punctuation. Currently this is probably English, hopefully we'll fix that in the future.", + "apierror-stashinvalidfile": "{{doc-apierror}}", "apierror-stashnosuchfilekey": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception text. Currently this is probably English, hopefully we'll fix that in the future.", "apierror-stashpathinvalid": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception text. Currently this is probably English, hopefully we'll fix that in the future.", "apierror-stashwrongowner": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception text, which should already end with punctuation. Currently this is probably English, hopefully we'll fix that in the future.", diff --git a/includes/exception/ErrorPageError.php b/includes/exception/ErrorPageError.php index 2f502d8294..9b5a26821f 100644 --- a/includes/exception/ErrorPageError.php +++ b/includes/exception/ErrorPageError.php @@ -24,7 +24,7 @@ * @since 1.7 * @ingroup Exception */ -class ErrorPageError extends MWException { +class ErrorPageError extends MWException implements ILocalizedException { public $title, $msg, $params; /** @@ -43,15 +43,23 @@ class ErrorPageError extends MWException { // customized by the local wiki. So get the default English version for // passing to the parent constructor. Our overridden report() below // makes sure that the page shown to the user is not forced to English. - if ( $msg instanceof Message ) { - $enMsg = clone $msg; - } else { - $enMsg = wfMessage( $msg, $params ); - } + $enMsg = $this->getMessageObject(); $enMsg->inLanguage( 'en' )->useDatabase( false ); parent::__construct( $enMsg->text() ); } + /** + * Return a Message object for this exception + * @since 1.29 + * @return Message + */ + public function getMessageObject() { + if ( $this->msg instanceof Message ) { + return clone $this->msg; + } + return wfMessage( $this->msg, $this->params ); + } + public function report() { global $wgOut; diff --git a/includes/exception/LocalizedException.php b/includes/exception/LocalizedException.php new file mode 100644 index 0000000000..cbdb53ef4f --- /dev/null +++ b/includes/exception/LocalizedException.php @@ -0,0 +1,66 @@ +messageSpec = $messageSpec; + + // Exception->getMessage() should be in plain English, not localized. + // So fetch the English version of the message, without local + // customizations, and make a basic attempt to turn markup into text. + $msg = $this->getMessageObject()->inLanguage( 'en' )->useDatabase( false )->text(); + $msg = preg_replace( '!!', '"', $msg ); + $msg = html_entity_decode( strip_tags( $msg ), ENT_QUOTES | ENT_HTML5 ); + parent::__construct( $msg, $code, $previous ); + } + + public function getMessageObject() { + return Message::newFromSpecifier( $this->messageSpec ); + } +} diff --git a/includes/exception/PermissionsError.php b/includes/exception/PermissionsError.php index e31374c2c7..5ecd237640 100644 --- a/includes/exception/PermissionsError.php +++ b/includes/exception/PermissionsError.php @@ -58,6 +58,9 @@ class PermissionsError extends ErrorPageError { } $this->errors = $errors; + + // Give the parent class something to work with + parent::__construct( 'permissionserrors', Message::newFromSpecifier( $errors[0] ) ); } public function report() { diff --git a/includes/libs/rdbms/exception/DBExpectedError.php b/includes/libs/rdbms/exception/DBExpectedError.php index 9e10884a59..7d303b1d8f 100644 --- a/includes/libs/rdbms/exception/DBExpectedError.php +++ b/includes/libs/rdbms/exception/DBExpectedError.php @@ -26,7 +26,7 @@ * @ingroup Database * @since 1.23 */ -class DBExpectedError extends DBError implements MessageSpecifier { +class DBExpectedError extends DBError implements MessageSpecifier, ILocalizedException { /** @var string[] Message parameters */ protected $params; @@ -42,4 +42,12 @@ class DBExpectedError extends DBError implements MessageSpecifier { public function getParams() { return $this->params; } + + /** + * @inheritdoc + * @since 1.29 + */ + public function getMessageObject() { + return Message::newFromSpecifier( $this ); + } } diff --git a/includes/title/MalformedTitleException.php b/includes/title/MalformedTitleException.php index 799961a424..2dddac5c2c 100644 --- a/includes/title/MalformedTitleException.php +++ b/includes/title/MalformedTitleException.php @@ -22,7 +22,7 @@ * MalformedTitleException is thrown when a TitleParser is unable to parse a title string. * @since 1.23 */ -class MalformedTitleException extends Exception { +class MalformedTitleException extends Exception implements ILocalizedException { private $titleText = null; private $errorMessage = null; private $errorMessageParameters = []; @@ -72,4 +72,12 @@ class MalformedTitleException extends Exception { public function getErrorMessageParameters() { return $this->errorMessageParameters; } + + /** + * @since 1.29 + * @return Message + */ + public function getMessageObject() { + return wfMessage( $this->getErrorMessage(), $this->getErrorMessageParameters() ); + } } -- 2.20.1