Contract for JSON encoding/decoding

[lenaorobei]

Problem

The use of JSON data format might be wider than just serialization and current Magento\Framework\Serialize\Serializer\Json does not cover all those needs.

Solution

New contract for JSON encoding/decoding.

Requested Reviewers

@maghamed
@Vinai
@fooman

Related Discussion

magento/magento-coding-standard#91

[Vinai]

In a nutshell, I'm for alternative solution 1 - keep the Json class and add an optional $option parameter. From a cost/benefit perspective that seems like the best solution to me.

Introducing a JsonEncoderInterface to me is using abstraction for a stable part of the system. Abstraction makes the most sense in areas that change a lot. The less something changes, the more concrete it should be.
In this case, chances are high there will never be an alternative Json implementation. It is coupled to the low level PHP function signature due to the options parameter syntax. For this reason I feel introducing a new interface is not required, in fact - it introduces more code when less code is sufficient. This violates Kent Beck's fourth rule of simple code: fewest possible elements.

Using virtual types is even more complex. I find that solution harder to understand as I would need to search di.xml to seem which options are set for a given type. It also puts additional weight on developers because they have to understand virtual types, arguably one of the more complex areas of object manager configuration. Keeping things simpler is usually better.

Personally I still will probably continue to use PHP native json_encode and json_decode functions when possible, but I do get the motivation behind wanting to protect the platform against vulnerabilities in code you don't control. For core development using the Json class seems like the right choice.
One of the things I struggle with when doing Magento development is that the whole system feels heavy and clunky, and isolating my own code against Magento by using native PHP functions and custom classes helps alleviate this.

[larsroettig]

I agree with @Vinai we should continue to use PHP native json_encode and json_decode functions. For me, I don't see a benefit to wrap native json.

[lenaorobei]

@Vinai @fooman @larsroettig @navarr @elevinskii
Would you mind attending next public architecture meeting #174 to discuss this topic?

[navarr]

I cannot attend the June 5th meeting due to prior engagements.

A summary of my standpoint is:

• As a frontend developer, we often need to utilize JSON for Magento's javascript initialization and configuration. I do not want additional overhead (e.g. the creation of a viewmodel) just to encode JSON.

[larsroettig]

I can also not attend but here my standpoint:

JSON is often used by Frontend developer to exchange Data with the Frontend. As navarr mention, it is additional overhead to warp JSON I don't see a benefit for wrapping it.

[Vinai]

I can attend the meeting.

[AlexMaxHorkun]

Removing SerializerInterface has been mentioned during the meeting - I don't think that's a good idea. There are situations where we just want to store complex data as a string and restore it later and we don't care for the format, for instance caching config values. If there's a problem with developers expecting SerializerInterface always to be Json we can remove the default preference and make developers to make a choice explicitly.

Regarding json options - would it be a bad things to add 'encrypt' and 'decrypt' methods to Json class?

[Vinai]

@AlexMaxHorkun Making the cache serialization format explicitly JSON enables the creation of tooling. Data is valuable, even in cache. No benefit in hiding the serialization format behind a SerializerInterface.

Regarding encrypt and decrypt - cryptography has absolutely nothing to do with JSON or serialization at all.

[ravmenon]

I also favor Other Possible Solutions - point 1.

Mostly from extension developer point-of view:

1. The existing SerializerInterface currently happens to be json, but tomorrow it could be some other scheme.
2. Extension developers may choose to use the Json implementation directly (point 1) which does have the API annotation, but they should still be allowed to use json_encode/json_decode directly too. For example, they maybe using json for HTTP service calls and so on, or for any custom purpose completely independent of how core Magento uses it for dependency injection, any other areas that require uses of SerializerInterface.

[melnikovi]

With option 1 I have small concern that options passed for one implementation may not be applicable or will cause unpredicted behavior for other implementations. This is why I like option 2 more.

[AlexMaxHorkun]

@AlexMaxHorkun Making the cache serialization format explicitly JSON enables the creation of tooling. Data is valuable, even in cache. No benefit in hiding the serialization format behind a SerializerInterface.

Regarding encrypt and decrypt - cryptography has absolutely nothing to do with JSON or serialization at all.

My mistake, I meant encode/decode.

Do we expect anybody creating child classes of Json serializer? If not I think adding optional parameters is good enough solution, albeit slightly messy one