Json::encode() and Json::decode() limit what we can do with JSON

I'm heavily on the fence on this one. And that can be painful, depending on the fence. I might have to jump to the -1 side for now.

I've done these checks:

• Applied the patch
• verified that there are no remaining instances of drupal_json_decode,
• checked basic json functionality within a test site
• ran json-based simpletests: all good

And then I started wondering about the asymmetry this leaves with json_encode though. Essentially your patch removes the wrapper from json_decode (which enforces one option), but it leaves the corresponding wrapper in place for json_encode:

function drupal_json_encode($var) {
  // Encode <, >, ', &, and " using the json_encode() options parameter.
  return json_encode($var, JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT);
}

I know I'm being finicky, but this just doesn't feel right (and not just because of the fence posts). If it's the number of options that dissuade you from removing the wrapper on json_encode, then maybe define the combined options as a constant somewhere and refer to that in the function calls. Your patch already converts many function calls to add an extra parameter (which increases the error-proneness - it's harder to maintain because of the number of instances that have to change if anything needs changing), so doing the same for the other wrapper shouldn't be a problem.

Actually the solution I'd prefer to see is further abstraction ... sorry :S ...

Thinking about the asymmetry of json_decode/drupal_json_encode and what the wrapper might be good for, I wonder if you might ever want to switch out the implementation of the json functions (either for some testing purpose, or because you need an option changed in all calls at once ... or you want to use a different library ... or for some unknown reason you want to transfer json as something else ... ok, going too far)

My point is:

• Keeping the wrappers around aids maintainability, in that options only need to be got right once, and they apply to every call
• If we remove one of them and keep the other, we end up with "lopsided" code. Not the worst thing in the world, but not ideal either
• If for some reason you want to change an option, or the underlying implementations, you have to change every instance in code, whereas with a wrapper of some sort you just change one line to change all calls.

That said, the existing wrappers don't gain us anything in flexibility, as the underlying implementation is hardcoded, the only thing they do gain us is keeping the options consistent across all calls.

I would personally suggest wrapping both in an object which manages the undelying implementation and which can be swapped out as a whole wherever needed. So, the first call to either would instantiate a swappable singleton which wraps both json_encode and json_decode with their appropriate options. I do have to wonder if this would be going too far though, for adding flexibility that has no concrete use case yet (AFAIK, no-one has ever asked to have the json backend swappable, the only obvious use case might be changing options).

Leaving in needs review for other opinions.

I would tend to agree that drupal_json_encode() and drupal_json_decode() should either stay or go as a pair...

And I'm also having trouble thinking of use cases where a particular Drupal site would need to swap out different JSON options (or the entire JSON implementation) across all the modules they're running at once. Doesn't mean it doesn't exist though :)

The thing is that both drupal_json_encode and drupal_json_decode are wrappers that fill in a non-default value for the second argument in their php counterpart. Removing the wrappers could bring about more clutter and inconsistent use of json_encode and json_decode in core.

Module developers have never been told that they have to use the drupal_ versions. If they wanted a different return value from the php counterparts, they probably used those native functions anyway.

Asking all implementing authors to update their module, however, is a big deal. If they don't get the memo, their modules will most likely break because objects are being treated as arrays. Keeping the wrappers in place has the added benefit that we could add extra functionality or security to them, making all implementing modules automatically receive 'the goodies'.

This patch looks like it cycles endlessly in testbot. :(

We require 5.5 for 8.x, this is a task so moving to 8.3.x. Likely needs tests to ensure we have this functionality persists

Anyone still interested in this thread?
The same limitations described apply equally to Drupal\Component\Serialization\Json, which hides all pass-through args to json functions.

Seems like it should be very simple and 100% backwards compatible to expose all the options in the component.
Any reason not to do this?

Tagging with API-First Initiative to give it more exposure. This looks not to give BC problems because the new parameters have default values.

I think that this might need additional test-coverage?

To be honest I'm not 100% following. This is a convenience wrapper for the common way how we want to encode/decode JSON. We know that these methods work perfectly together as in, one is the opposite of the other.

What I don't fully understand is the following: What do we gain by adding these parameters? If someone wants to not do the default Drupal json encoding, aka. when they use json_encode/json_decode, they put a quite semantic statement in their code: We use the non Drupal function as we do something different.

If we want to support that usecase, how about adding encodeWithOptions and decodeWithoptions. That way we don't change the signature of \Drupal\Component\Serialization\SerializationInterface.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Also not sure about the "why" for this.

Tagging for an issue summary update to better explain why this needs to be done.

Also see it was previously tagged for tests which still would need to happen.

Thanks!