Currently, phutil_json_decode can only decode objects ({}) or arrays ([]). This makes it somewhat incompatible with the regular json_decode function. In particular, see rP31a89bb94d2af882953490cf23e5fc928520d598, rP5f7ee505b6b71faf7e7ec82ce2ff6c0acb304a82 and rP336ccfeea12eded8980716c3b65833f921705d98.
Details
Diff Detail
- Repository
- rPHU libphutil
- Branch
- master
- Lint
Lint Errors Severity Location Code Message Error src/utils/utils.php:1080 XHP45 PHP Compatibility Error src/utils/utils.php:1080 XHP45 PHP Compatibility - Unit
Tests Passed - Build Status
Buildable 5782 Build 5801: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
I think it's desirable that this doesn't decode scalars.
Almost all the places where we decode should always be dictionaries. If we make this change, we need to go to every callsite and do this:
try { $user_config = phutil_json_decode($user_config_data); + if (!is_array($user_config)) { + throw new Exception( + pht( + "Your '~/.arcrc' file is a valid JSON scalar, but must be a JSON object, ". + "not a scalar.")); + } } catch (PhutilJSONParserException $ex) { throw new PhutilProxyException( "Your '~/.arcrc' file is not a valid JSON file.", $ex); }
If we don't do this, we'll often fatal in a much more confusing way as soon as we try to idx() the value or do something else array-like with it.
In previous diffs, you replaced something like 50 callsites, and we had to revert 4 of them, so approximately ~90%+ of our use is decoding "real" JSON objects.
If anything, I'd say we could change the signature to:
phutil_json_decode($data, $optional_source_description = null);
...so we could do something like this:
- try { - $user_config = phutil_json_decode($user_config_data); + $user_config = phutil_json_decode( + $user_config_data, + pht('Your "~/.arcrc" file is not valid JSON.')); - } catch (PhutilJSONParserException $ex) { - throw new PhutilProxyException( - "Your '~/.arcrc' file is not a valid JSON file.", - $ex); - }
I think that your points are valid, but generally I think that phutil_json_decode should essentially just be "json_decode with exceptions". If we wanted a convenience wrapper function then we could create phutil_json_decode_array.
I'm happy to just leave the behavior as-is. I do agree that most of the time when we are parsing JSON we are expecting an array, so I think that the current behavior is correct/desirable.
I actually agree with you but don't want to bother converting everything again myself or make you do it. :P