Page MenuHomePhabricator

Modify the JsonLint library to return `array` instead of `stdClass`.
ClosedPublic

Authored by joshuaspence on Jun 18 2014, 6:15 PM.
Tags
None
Referenced Files
F14042978: D9623.diff
Tue, Nov 12, 7:09 AM
F14028532: D9623.diff
Fri, Nov 8, 2:02 PM
F14025400: D9623.diff
Thu, Nov 7, 5:17 PM
F14006914: D9623.id23081.diff
Mon, Oct 28, 9:02 PM
F13984515: D9623.id23107.diff
Sun, Oct 20, 1:57 PM
F13972627: D9623.id23081.diff
Thu, Oct 17, 8:09 PM
F13968079: D9623.id23104.diff
Wed, Oct 16, 6:26 PM
F13967834: D9623.id23111.diff
Wed, Oct 16, 4:19 PM
Subscribers

Details

Summary

Summary :Ref T5297. Make PhutilJSONParser::parse($json) roughly approximate to json_decode($json, true). Generally when we parse JSON, we want the result to be an arrray, not a stdClass object.

Depends on D9622.

Test Plan

Wrote a few additional test cases.

Diff Detail

Repository
rPHU libphutil
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Modify the JsonLint library to return `array` instead of `stdClass`..
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence updated this object.
epriestley edited edge metadata.
This revision is now accepted and ready to land.Jun 19 2014, 6:01 PM

It turns out this fails for some more complex test cases. I am fixing it now.

joshuaspence edited edge metadata.
  • Minor fix
  • Additional test cases
  • Make it actually work as it is intended.
This revision is now accepted and ready to land.Jun 19 2014, 9:57 PM
joshuaspence edited edge metadata.
epriestley edited edge metadata.

Hmm, the empty string handling seems funky. Does that get cleaned up somewhere non-obvious? We should probably have a test case for it, regardless, since it has magical logic.

externals/jsonlint/src/Seld/JsonLint/JsonParser.php
361

Is it practical/easier to fix this to array()? (And on line 374)?

371

This _empty_ stuff seems wrong. {"":1} should parse like array('' => 1), but I'm guessing we end up with _empty_ on the way out of the parser instead?

This revision now requires changes to proceed.Jun 19 2014, 11:08 PM

Hmm, the empty string handling seems funky. Does that get cleaned up somewhere non-obvious? We should probably have a test case for it, regardless, since it has magical logic.

The other way would be to fix JsonLintLexer. If you pass an empty string into JsonLintJsonParser, the following warning is raised:

ERROR 2: str_repeat(): Second argument has to be greater than or equal to 0 at [/home/joshua/workspace/github.com/phacility/libphutil/externals/jsonlint/src/Seld/JsonLint/Lexer.php:82]
externals/jsonlint/src/Seld/JsonLint/JsonParser.php
361

This was my initial approach... I think the issue here was that I wasn't able to later differentiate if something was a JSON object or a JSON array.

371

Yeah I agree... I don't know why they have done this. I imagine we will strip this out.

I wasn't able to later differentiate if something was a JSON object or a JSON array.

Hrrm, is this important?

If we want to retain the ability to distinguish between the two, we should probably export the stdClass from the parser, then recursively walk applying (array) casts it if some option is set.

I'm guessing the _empty_ junk is because PHP might not allow the empty string in property names, though:

$ php -r '$empty = ""; $o = (object)array(); $o->$empty = "x"; print_r($o);'
PHP Fatal error:  Cannot access empty property in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0

I guess we could also define class JSONNode implements ArrayAccess, and have that support both empty strings and some object vs array flag, to solve both problems. But: ugh, gross.

I'd suggest:

  • For now, don't worry about distinguishing, and just always use array()?
  • If we have a need to distinguish later, we can add a mode flag and implement something like JSONNode to work around the empty property issue?

I wasn't able to later differentiate if something was a JSON object or a JSON array.

Hrrm, is this important?

If we want to retain the ability to distinguish between the two, we should probably export the stdClass from the parser, then recursively walk applying (array) casts it if some option is set.

I'm guessing the _empty_ junk is because PHP might not allow the empty string in property names, though:

$ php -r '$empty = ""; $o = (object)array(); $o->$empty = "x"; print_r($o);'
PHP Fatal error:  Cannot access empty property in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0

I guess we could also define class JSONNode implements ArrayAccess, and have that support both empty strings and some object vs array flag, to solve both problems. But: ugh, gross.

I'd suggest:

  • For now, don't worry about distinguishing, and just always use array()?
  • If we have a need to distinguish later, we can add a mode flag and implement something like JSONNode to work around the empty property issue?

No, I actually couldn't get the unit tests passing when using array() instead of new stdClass. Perhaps I was missing something though. Let me try again

joshuaspence edited edge metadata.
  • Use array instead of stdClass

This doesn't work... and I'm not sure why:

Assertion failed, expected values to be equal (at PhutilJSONParserTestCase.php:24): Parsing JSON: {"foo": "bar", "bar": ["baz"]}
Expected vs Actual Output Diff
--- Old Value
+++ New Value
@@ -1,10 +1,5 @@
 Array
 (
     [foo] => bar
-    [bar] => Array
-        (
-            [0] => baz
-        )
-
 )

I think I got it -- that was tricky!

externals/jsonlint/src/Seld/JsonLint/JsonParser.php
386

(This should be an index access now.)

389

This is the actual bug, and should be $yyval->token[$key] = ....

Since the thing is an array now, it gets copy-on-write semantics. Previously, as an object, it had reference semantics.

Also, the isset() checks should probably be array_key_exists($key, $array) checks. I'd expect them to work incorrectly when the value is null. I think that's an existing bug with the upstream implementation.

joshuaspence edited edge metadata.
  • Fix the strange behavior for handling empty strings.
epriestley edited edge metadata.
epriestley added inline comments.
externals/jsonlint/src/Seld/JsonLint/JsonParser.php
386

This should probably become array_key_exists() as well.

This revision is now accepted and ready to land.Jun 20 2014, 12:18 AM