From 5a166f00d8bb979074a30d4aaa6485c7e19fd100 Mon Sep 17 00:00:00 2001
From: Bill Pirkle <bpirkle@wikimedia.org>
Date: Wed, 7 Nov 2018 13:04:21 -0600
Subject: [PATCH] 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
---
 includes/CommentStore.php                     |  5 +-
 includes/json/FormatJson.php                  | 10 ++++
 .../phpunit/includes/json/FormatJsonTest.php  | 54 ++++++++++++++++++-
 3 files changed, 65 insertions(+), 4 deletions(-)

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} );
+		}
+	}
 }
-- 
2.20.1