From b0784a8e964c4ad435b5e9bc88393a89dbabcf75 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Wed, 9 Nov 2016 00:42:49 +0000 Subject: [PATCH] Deprecate Message::$format (mostly) Message::__toString() used the same formatting mode that the last explicit transformation used: $msg = new Message( 'foo' ); echo $msg; // escaped echo $msg->plain(); echo $msg; // not escaped This is not particularly useful and makes code review hard, so let's get rid of it. The same behavior with $msg->toString() is left intact (and logged) for now. Bug: T146416 Change-Id: Ia9b2a1dcf09d52348b2c6d8299fd849b809f6e74 --- includes/Message.php | 106 ++++++++++-------- tests/phpunit/includes/MessageTest.php | 42 ++++--- tests/phpunit/includes/api/ApiMessageTest.php | 4 +- 3 files changed, 84 insertions(+), 68 deletions(-) diff --git a/includes/Message.php b/includes/Message.php index c1a12aa912..27aab8ad5f 100644 --- a/includes/Message.php +++ b/includes/Message.php @@ -157,6 +157,16 @@ * @since 1.17 */ class Message implements MessageSpecifier, Serializable { + /** Use message text as-is */ + const FORMAT_PLAIN = 'plain'; + /** Use normal wikitext -> HTML parsing (the result will be wrapped in a block-level HTML tag) */ + const FORMAT_BLOCK_PARSE = 'block-parse'; + /** Use normal wikitext -> HTML parsing but strip the block-level wrapper */ + const FORMAT_PARSE = 'parse'; + /** Transform {{..}} constructs but don't transform to HTML */ + const FORMAT_TEXT = 'text'; + /** Transform {{..}} constructs, HTML-escape the result */ + const FORMAT_ESCAPED = 'escaped'; /** * In which language to get this message. True, which is the default, @@ -190,15 +200,8 @@ class Message implements MessageSpecifier, Serializable { protected $parameters = []; /** - * Format for the message. - * Supported formats are: - * * text (transform) - * * escaped (transform+htmlspecialchars) - * * block-parse - * * parse (default) - * * plain - * * @var string + * @deprecated */ protected $format = 'parse'; @@ -347,8 +350,10 @@ class Message implements MessageSpecifier, Serializable { * @since 1.21 * * @return string + * @deprecated since 1.29 formatting is not stateful */ public function getFormat() { + wfDeprecated( __METHOD__, '1.29' ); return $this->format; } @@ -796,9 +801,18 @@ class Message implements MessageSpecifier, Serializable { * * @since 1.17 * + * @param string|null $format One of the FORMAT_* constants. Null means use whatever was used + * the last time (this is for B/C and should be avoided). + * * @return string HTML */ - public function toString() { + public function toString( $format = null ) { + if ( $format === null ) { + $ex = new LogicException( __METHOD__ . ' using implicit format: ' . $this->format ); + \MediaWiki\Logger\LoggerFactory::getInstance( 'message-format' )->warning( + $ex->getMessage(), [ 'exception' => $ex, 'format' => $this->format, 'key' => $this->key ] ); + $format = $this->format; + } $string = $this->fetchMessage(); if ( $string === false ) { @@ -821,23 +835,23 @@ class Message implements MessageSpecifier, Serializable { } # Replace parameters before text parsing - $string = $this->replaceParameters( $string, 'before' ); + $string = $this->replaceParameters( $string, 'before', $format ); # Maybe transform using the full parser - if ( $this->format === 'parse' ) { + if ( $format === self::FORMAT_PARSE ) { $string = $this->parseText( $string ); $string = Parser::stripOuterParagraph( $string ); - } elseif ( $this->format === 'block-parse' ) { + } elseif ( $format === self::FORMAT_BLOCK_PARSE ) { $string = $this->parseText( $string ); - } elseif ( $this->format === 'text' ) { + } elseif ( $format === self::FORMAT_TEXT ) { $string = $this->transformText( $string ); - } elseif ( $this->format === 'escaped' ) { + } elseif ( $format === self::FORMAT_ESCAPED ) { $string = $this->transformText( $string ); $string = htmlspecialchars( $string, ENT_QUOTES, 'UTF-8', false ); } # Raw parameter replacement - $string = $this->replaceParameters( $string, 'after' ); + $string = $this->replaceParameters( $string, 'after', $format ); return $string; } @@ -852,17 +866,11 @@ class Message implements MessageSpecifier, Serializable { * @return string */ public function __toString() { - if ( $this->format !== 'parse' ) { - $ex = new LogicException( __METHOD__ . ' using implicit format: ' . $this->format ); - \MediaWiki\Logger\LoggerFactory::getInstance( 'message-format' )->warning( - $ex->getMessage(), [ 'exception' => $ex, 'format' => $this->format, 'key' => $this->key ] ); - } - // PHP doesn't allow __toString to throw exceptions and will // trigger a fatal error if it does. So, catch any exceptions. try { - return $this->toString(); + return $this->toString( self::FORMAT_PARSE ); } catch ( Exception $ex ) { try { trigger_error( "Exception caught in " . __METHOD__ . " (message " . $this->key . "): " @@ -871,10 +879,7 @@ class Message implements MessageSpecifier, Serializable { // Doh! Cause a fatal error after all? } - if ( $this->format === 'plain' || $this->format === 'text' ) { - return '<' . $this->key . '>'; - } - return '<' . htmlspecialchars( $this->key ) . '>'; + return '⧼' . htmlspecialchars( $this->key ) . '⧽'; } } @@ -886,8 +891,8 @@ class Message implements MessageSpecifier, Serializable { * @return string Parsed HTML. */ public function parse() { - $this->format = 'parse'; - return $this->toString(); + $this->format = self::FORMAT_PARSE; + return $this->toString( self::FORMAT_PARSE ); } /** @@ -898,8 +903,8 @@ class Message implements MessageSpecifier, Serializable { * @return string Unescaped message text. */ public function text() { - $this->format = 'text'; - return $this->toString(); + $this->format = self::FORMAT_TEXT; + return $this->toString( self::FORMAT_TEXT ); } /** @@ -910,8 +915,8 @@ class Message implements MessageSpecifier, Serializable { * @return string Unescaped untransformed message text. */ public function plain() { - $this->format = 'plain'; - return $this->toString(); + $this->format = self::FORMAT_PLAIN; + return $this->toString( self::FORMAT_PLAIN ); } /** @@ -922,8 +927,8 @@ class Message implements MessageSpecifier, Serializable { * @return string HTML */ public function parseAsBlock() { - $this->format = 'block-parse'; - return $this->toString(); + $this->format = self::FORMAT_BLOCK_PARSE; + return $this->toString( self::FORMAT_BLOCK_PARSE ); } /** @@ -935,8 +940,8 @@ class Message implements MessageSpecifier, Serializable { * @return string Escaped message text. */ public function escaped() { - $this->format = 'escaped'; - return $this->toString(); + $this->format = self::FORMAT_ESCAPED; + return $this->toString( self::FORMAT_ESCAPED ); } /** @@ -1070,13 +1075,14 @@ class Message implements MessageSpecifier, Serializable { * * @param string $message The message text. * @param string $type Either "before" or "after". + * @param string $format One of the FORMAT_* constants. * * @return string */ - protected function replaceParameters( $message, $type = 'before' ) { + protected function replaceParameters( $message, $type = 'before', $format ) { $replacementKeys = []; foreach ( $this->parameters as $n => $param ) { - list( $paramType, $value ) = $this->extractParam( $param ); + list( $paramType, $value ) = $this->extractParam( $param, $format ); if ( $type === $paramType ) { $replacementKeys['$' . ( $n + 1 )] = $value; } @@ -1091,10 +1097,11 @@ class Message implements MessageSpecifier, Serializable { * @since 1.18 * * @param mixed $param Parameter as defined in this class. + * @param string $format One of the FORMAT_* constants. * * @return array Array with the parameter type (either "before" or "after") and the value. */ - protected function extractParam( $param ) { + protected function extractParam( $param, $format ) { if ( is_array( $param ) ) { if ( isset( $param['raw'] ) ) { return [ 'after', $param['raw'] ]; @@ -1113,7 +1120,7 @@ class Message implements MessageSpecifier, Serializable { } elseif ( isset( $param['bitrate'] ) ) { return [ 'before', $this->getLanguage()->formatBitrate( $param['bitrate'] ) ]; } elseif ( isset( $param['plaintext'] ) ) { - return [ 'after', $this->formatPlaintext( $param['plaintext'] ) ]; + return [ 'after', $this->formatPlaintext( $param['plaintext'], $format ) ]; } else { $warning = 'Invalid parameter for message "' . $this->getKey() . '": ' . htmlspecialchars( serialize( $param ) ); @@ -1127,7 +1134,7 @@ class Message implements MessageSpecifier, Serializable { // Message objects should not be before parameters because // then they'll get double escaped. If the message needs to be // escaped, it'll happen right here when we call toString(). - return [ 'after', $param->toString() ]; + return [ 'after', $param->toString( $format ) ]; } else { return [ 'before', $param ]; } @@ -1207,18 +1214,19 @@ class Message implements MessageSpecifier, Serializable { * @since 1.25 * * @param string $plaintext String to ensure plaintext output of + * @param string $format One of the FORMAT_* constants. * - * @return string Input plaintext encoded for output to $this->format + * @return string Input plaintext encoded for output to $format */ - protected function formatPlaintext( $plaintext ) { - switch ( $this->format ) { - case 'text': - case 'plain': + protected function formatPlaintext( $plaintext, $format ) { + switch ( $format ) { + case self::FORMAT_TEXT: + case self::FORMAT_PLAIN: return $plaintext; - case 'parse': - case 'block-parse': - case 'escaped': + case self::FORMAT_PARSE: + case self::FORMAT_BLOCK_PARSE: + case self::FORMAT_ESCAPED: default: return htmlspecialchars( $plaintext, ENT_QUOTES ); diff --git a/tests/phpunit/includes/MessageTest.php b/tests/phpunit/includes/MessageTest.php index 9b9a73a886..bb9af8f204 100644 --- a/tests/phpunit/includes/MessageTest.php +++ b/tests/phpunit/includes/MessageTest.php @@ -236,11 +236,14 @@ class MessageTest extends MediaWikiLangTestCase { public static function provideToString() { return [ - [ 'mainpage', 'Main Page' ], - [ 'i-dont-exist-evar', '⧼i-dont-exist-evar⧽' ], - [ 'i-dont-exist-evar', '⧼i-dont-exist-evar⧽', 'escaped' ], - [ 'script>alert(1)alert(1)alert(1)alert(1)$format(); + $this->assertEquals( $expect, $msg->$format() ); + $this->assertEquals( $expect, $msg->toString() ); + $this->assertEquals( $expectImplicit, $msg->__toString() ); $this->assertEquals( $expect, $msg->toString() ); - $this->assertEquals( $expect, $msg->__toString() ); } public static function provideToString_raw() { return [ - [ 'foo', 'foo', 'parse' ], - [ 'foo', '<span>foo</span>', 'escaped' ], - [ 'foo', 'foo', 'plain' ], - [ '', '<script>alert(1)</script>', 'parse' ], - [ '', '<script>alert(1)</script>', 'escaped' ], - [ '', '', 'plain' ], + [ 'foo', 'parse', 'foo', 'foo' ], + [ 'foo', 'escaped', '<span>foo</span>', + 'foo' ], + [ 'foo', 'plain', 'foo', 'foo' ], + [ '', 'parse', '<script>alert(1)</script>', + '<script>alert(1)</script>' ], + [ '', 'escaped', '<script>alert(1)</script>', + '<script>alert(1)</script>' ], + [ '', 'plain', '', + '<script>alert(1)</script>' ], ]; } @@ -272,16 +280,16 @@ class MessageTest extends MediaWikiLangTestCase { * @covers Message::__toString * @dataProvider provideToString_raw */ - public function testToString_raw( $key, $expect, $format ) { + public function testToString_raw( $key, $format, $expect, $expectImplicit ) { // make the message behave like RawMessage and use the key as-is $msg = $this->getMockBuilder( Message::class )->setMethods( [ 'fetchMessage' ] ) ->setConstructorArgs( [ $key ] ) ->getMock(); $msg->expects( $this->any() )->method( 'fetchMessage' )->willReturn( $key ); /** @var Message $msg */ - $msg->$format(); + $this->assertEquals( $expect, $msg->$format() ); $this->assertEquals( $expect, $msg->toString() ); - $this->assertEquals( $expect, $msg->__toString() ); + $this->assertEquals( $expectImplicit, $msg->__toString() ); $this->assertEquals( $expect, $msg->toString() ); } diff --git a/tests/phpunit/includes/api/ApiMessageTest.php b/tests/phpunit/includes/api/ApiMessageTest.php index a45015a7b3..8764b4194f 100644 --- a/tests/phpunit/includes/api/ApiMessageTest.php +++ b/tests/phpunit/includes/api/ApiMessageTest.php @@ -5,17 +5,17 @@ */ class ApiMessageTest extends MediaWikiTestCase { - private function compareMessages( $msg, $msg2 ) { + private function compareMessages( Message $msg, Message $msg2 ) { $this->assertSame( $msg->getKey(), $msg2->getKey(), 'getKey' ); $this->assertSame( $msg->getKeysToTry(), $msg2->getKeysToTry(), 'getKeysToTry' ); $this->assertSame( $msg->getParams(), $msg2->getParams(), 'getParams' ); - $this->assertSame( $msg->getFormat(), $msg2->getFormat(), 'getFormat' ); $this->assertSame( $msg->getLanguage(), $msg2->getLanguage(), 'getLanguage' ); $msg = TestingAccessWrapper::newFromObject( $msg ); $msg2 = TestingAccessWrapper::newFromObject( $msg2 ); $this->assertSame( $msg->interface, $msg2->interface, 'interface' ); $this->assertSame( $msg->useDatabase, $msg2->useDatabase, 'useDatabase' ); + $this->assertSame( $msg->format, $msg2->format, 'format' ); $this->assertSame( $msg->title ? $msg->title->getFullText() : null, $msg2->title ? $msg2->title->getFullText() : null, -- 2.20.1