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 1be7951..7a2726f 100644 (file)
--- 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 fbcb3bd..9ec3d96 100644 (file)
--- 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 a4ab879..2760cb9 100644 (file)
--- 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} );
+               }
+       }
 }