Page MenuHomePhabricator

Make phutil_json_decode more powerful
AbandonedPublic

Authored by joshuaspence on May 5 2015, 8:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 6, 2:33 AM
Unknown Object (File)
Wed, Nov 27, 6:59 AM
Unknown Object (File)
Mon, Nov 25, 6:00 PM
Unknown Object (File)
Sun, Nov 24, 2:31 AM
Unknown Object (File)
Sat, Nov 23, 6:24 AM
Unknown Object (File)
Nov 19 2024, 12:43 AM
Unknown Object (File)
Nov 15 2024, 2:52 AM
Unknown Object (File)
Nov 10 2024, 4:25 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

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.

Test Plan

Expanded unit tests.

Diff Detail

Repository
rPHU libphutil
Branch
master
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/utils/utils.php:1080XHP45PHP Compatibility
Errorsrc/utils/utils.php:1080XHP45PHP Compatibility
Unit
Tests Passed
Build Status
Buildable 5782
Build 5801: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Make phutil_json_decode more powerful.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

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