From 5f01cbb3ad0676a4b6e3a7d33d4181558c92d3d7 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 21 Jun 2016 11:12:53 -0400 Subject: [PATCH] Allow Message::newFromSpecifier to handle ApiMessages Instead of constructing a new Message from the Message as a MessageSpecifier, just clone the existing Message which will preserve subclass data. Also, make use of this to simplify the logic in ApiBase::parseMsg(). Change-Id: I9545acb8da752c0c21e16d8b1d37d8802fcb329d --- includes/Message.php | 4 +-- includes/api/ApiBase.php | 40 +++++++++++++++----------- includes/api/ApiUpload.php | 2 +- tests/phpunit/includes/MessageTest.php | 5 ++++ 4 files changed, 31 insertions(+), 20 deletions(-) diff --git a/includes/Message.php b/includes/Message.php index 712d3f17fb..d0325d79b8 100644 --- a/includes/Message.php +++ b/includes/Message.php @@ -402,8 +402,8 @@ class Message implements MessageSpecifier, Serializable { $value = array_shift( $params ); } - if ( $value instanceof RawMessage ) { - $message = new RawMessage( $value->getKey(), $value->getParams() ); + if ( $value instanceof Message ) { // Message, RawMessage, ApiMessage, etc + $message = clone( $value ); } elseif ( $value instanceof MessageSpecifier ) { $message = new Message( $value ); } elseif ( is_string( $value ) ) { diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index 639f6be0b8..3e57e8997a 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -2093,7 +2093,7 @@ abstract class ApiBase extends ContextSource { /** * Output the error message related to a certain array - * @param array|string $error Element of a getUserPermissionsErrors()-style array + * @param array|string|MessageSpecifier $error Element of a getUserPermissionsErrors()-style array * @throws UsageException always */ public function dieUsageMsg( $error ) { @@ -2110,7 +2110,7 @@ abstract class ApiBase extends ContextSource { /** * Will only set a warning instead of failing if the global $wgDebugAPI * is set to true. Otherwise behaves exactly as dieUsageMsg(). - * @param array|string $error Element of a getUserPermissionsErrors()-style array + * @param array|string|MessageSpecifier $error Element of a getUserPermissionsErrors()-style array * @throws UsageException * @since 1.21 */ @@ -2143,32 +2143,38 @@ abstract class ApiBase extends ContextSource { /** * Return the error message related to a certain array - * @param array $error Element of a getUserPermissionsErrors()-style array + * @param array|string|MessageSpecifier $error Element of a getUserPermissionsErrors()-style array * @return array('code' => code, 'info' => info) */ public function parseMsg( $error ) { - $error = (array)$error; // It seems strings sometimes make their way in here - $key = array_shift( $error ); - - // Check whether the error array was nested - // array( array( , ), array( , ) ) - if ( is_array( $key ) ) { - $error = $key; - $key = array_shift( $error ); + // Check whether someone passed the whole array, instead of one element as + // documented. This breaks if it's actually an array of fallback keys, but + // that's long-standing misbehavior introduced in r87627 to incorrectly + // fix T30797. + if ( is_array( $error ) ) { + $first = reset( $error ); + if ( is_array( $first ) ) { + wfDebug( __METHOD__ . ' was passed an array of arrays. ' . wfGetAllCallers( 5 ) ); + $error = $first; + } } - if ( $key instanceof IApiMessage ) { + $msg = Message::newFromSpecifier( $error ); + + if ( $msg instanceof IApiMessage ) { return [ - 'code' => $key->getApiCode(), - 'info' => $key->inLanguage( 'en' )->useDatabase( false )->text(), - 'data' => $key->getApiData() + 'code' => $msg->getApiCode(), + 'info' => $msg->inLanguage( 'en' )->useDatabase( false )->text(), + 'data' => $msg->getApiData() ]; } + $key = $msg->getKey(); if ( isset( self::$messageMap[$key] ) ) { + $params = $msg->getParams(); return [ - 'code' => wfMsgReplaceArgs( self::$messageMap[$key]['code'], $error ), - 'info' => wfMsgReplaceArgs( self::$messageMap[$key]['info'], $error ) + 'code' => wfMsgReplaceArgs( self::$messageMap[$key]['code'], $params ), + 'info' => wfMsgReplaceArgs( self::$messageMap[$key]['info'], $params ) ]; } diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php index 0a79aa496f..15c1e39505 100644 --- a/includes/api/ApiUpload.php +++ b/includes/api/ApiUpload.php @@ -347,7 +347,7 @@ class ApiUpload extends ApiBase { * Throw an error that the user can recover from by providing a better * value for $parameter * - * @param array $error Error array suitable for passing to dieUsageMsg() + * @param array|string|MessageSpecifier $error Error suitable for passing to dieUsageMsg() * @param string $parameter Parameter that needs revising * @param array $data Optional extra data to pass to the user * @throws UsageException diff --git a/tests/phpunit/includes/MessageTest.php b/tests/phpunit/includes/MessageTest.php index 8aa136144d..c4f3fb1497 100644 --- a/tests/phpunit/includes/MessageTest.php +++ b/tests/phpunit/includes/MessageTest.php @@ -589,6 +589,10 @@ class MessageTest extends MediaWikiLangTestCase { public function testNewFromSpecifier( $value, $expectedText ) { $message = Message::newFromSpecifier( $value ); $this->assertInstanceOf( Message::class, $message ); + if ( $value instanceof Message ) { + $this->assertInstanceOf( get_class( $value ), $message ); + $this->assertEquals( $value, $message ); + } $this->assertSame( $expectedText, $message->text() ); } @@ -602,6 +606,7 @@ class MessageTest extends MediaWikiLangTestCase { 'array' => [ [ 'youhavenewmessages', 'foo', 'bar' ], 'You have foo (bar).' ], 'Message' => [ new Message( 'youhavenewmessages', [ 'foo', 'bar' ] ), 'You have foo (bar).' ], 'RawMessage' => [ new RawMessage( 'foo ($1)', [ 'bar' ] ), 'foo (bar)' ], + 'ApiMessage' => [ new ApiMessage( [ 'mainpage' ], 'code', [ 'data' ] ), 'Main Page' ], 'MessageSpecifier' => [ $messageSpecifier, 'Main Page' ], 'nested RawMessage' => [ [ new RawMessage( 'foo ($1)', [ 'bar' ] ) ], 'foo (bar)' ], ]; -- 2.20.1