From: Bill Pirkle <bpirkle@wikimedia.org>
Date: Wed, 7 Nov 2018 19:04:21 +0000 (-0600)
Subject: Comments, tests, and tweaks for JSON decoding quirks
X-Git-Tag: 1.34.0-rc.0~3528^2
X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;ds=sidebyside;h=5a166f00d8bb979074a30d4aaa6485c7e19fd100;p=lhc%2Fweb%2Fwiklou.git

Comments, tests, and tweaks for JSON decoding quirks

PHP JSON decoding has surprising behavior on some edge cases.
Documented this via comments, added related tests, and tweaked
related CommentStore code.

Bug: T206411
Change-Id: I6927fdaf616b37a04d81a638a0ed257afac9b844
---

diff --git a/includes/CommentStore.php b/includes/CommentStore.php
index 1be79510cb..7a2726f291 100644
--- a/includes/CommentStore.php
+++ b/includes/CommentStore.php
@@ -349,14 +349,13 @@ class CommentStore {
 
 		$msg = null;
 		if ( $data !== null ) {
-			$data = FormatJson::decode( $data );
-			if ( !is_object( $data ) ) {
+			$data = FormatJson::decode( $data, true );
+			if ( !is_array( $data ) ) {
 				// @codeCoverageIgnoreStart
 				wfLogWarning( "Invalid JSON object in comment: $data" );
 				$data = null;
 				// @codeCoverageIgnoreEnd
 			} else {
-				$data = (array)$data;
 				if ( isset( $data['_message'] ) ) {
 					$msg = self::decodeMessage( $data['_message'] )
 						->setInterfaceMessageFlag( true );
diff --git a/includes/json/FormatJson.php b/includes/json/FormatJson.php
index fbcb3bd3b8..9ec3d96002 100644
--- a/includes/json/FormatJson.php
+++ b/includes/json/FormatJson.php
@@ -153,6 +153,16 @@ class FormatJson {
 	 * which returns more comprehensive result in case of an error, and has
 	 * more parsing options.
 	 *
+	 * In PHP versions before 7.1, decoding a JSON string containing an empty key
+	 * without passing $assoc as true results in a return object with a property
+	 * named "_empty_" (because true empty properties were not supported pre-PHP-7.1).
+	 * Instead, consider passing $assoc as true to return an associative array.
+	 *
+	 * But be aware that in all supported PHP versions, decoding an empty JSON object
+	 * with $assoc = true returns an array, not an object, breaking round-trip consistency.
+	 *
+	 * See https://phabricator.wikimedia.org/T206411 for more details on these quirks.
+	 *
 	 * @param string $value The JSON string being decoded
 	 * @param bool $assoc When true, returned objects will be converted into associative arrays.
 	 *
diff --git a/tests/phpunit/includes/json/FormatJsonTest.php b/tests/phpunit/includes/json/FormatJsonTest.php
index a4ab879ffb..2760cb9f22 100644
--- a/tests/phpunit/includes/json/FormatJsonTest.php
+++ b/tests/phpunit/includes/json/FormatJsonTest.php
@@ -172,7 +172,7 @@ class FormatJsonTest extends MediaWikiTestCase {
 	/**
 	 * Test data for testParseTryFixing.
 	 *
-	 * Some PHP interpreters use json-c rather than the JSON.org cannonical
+	 * Some PHP interpreters use json-c rather than the JSON.org canonical
 	 * parser to avoid being encumbered by the "shall be used for Good, not
 	 * Evil" clause of the JSON.org parser's license. By default, json-c
 	 * parses in a non-strict mode which allows trailing commas for array and
@@ -372,4 +372,56 @@ class FormatJsonTest extends MediaWikiTestCase {
 
 		return $cases;
 	}
+
+	public function provideEmptyJsonKeyStrings() {
+		return [
+			[
+				'{"":"foo"}',
+				'{"":"foo"}',
+				''
+			],
+			[
+				'{"_empty_":"foo"}',
+				'{"_empty_":"foo"}',
+				'_empty_' ],
+			[
+				'{"\u005F\u0065\u006D\u0070\u0074\u0079\u005F":"foo"}',
+				'{"_empty_":"foo"}',
+				'_empty_'
+			],
+			[
+				'{"_empty_":"bar","":"foo"}',
+				'{"_empty_":"bar","":"foo"}',
+				''
+			],
+			[
+				'{"":"bar","_empty_":"foo"}',
+				'{"":"bar","_empty_":"foo"}',
+				'_empty_'
+			]
+		];
+	}
+
+	/**
+	 * @covers FormatJson::encode
+	 * @covers FormatJson::decode
+	 * @dataProvider provideEmptyJsonKeyStrings
+	 * @param string $json
+	 *
+	 * Decoding behavior with empty keys can be surprising.
+	 * See https://phabricator.wikimedia.org/T206411
+	 */
+	public function testEmptyJsonKeyArray( $json, $expect, $php71Name ) {
+		// Decoding to array is consistent across supported PHP versions
+		$this->assertSame( $expect, FormatJson::encode(
+			FormatJson::decode( $json, true ) ) );
+
+		// Decoding to object differs between supported PHP versions
+		$obj = FormatJson::decode( $json );
+		if ( version_compare( PHP_VERSION, '7.1', '<' ) ) {
+			$this->assertEquals( 'foo', $obj->_empty_ );
+		} else {
+			$this->assertEquals( 'foo', $obj->{$php71Name} );
+		}
+	}
 }