Contract for JSON encoding/decoding

[lenaorobei]

Since I forgot about #161 and force-pushed to my master, creating new PR.

Hopefully this PR will get the second wind 🙂

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

#161
magento/magento-coding-standard#91

[Vinai]

My previous #161 (comment) still applies:

---

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.

---

[navarr]

An interface might be useful for alternative implementations - such as JSON5

[Vinai]

@navarr The point of using a known interchangeable serialization format like JSON is that external tooling besides Magento can use the data. It needs to be dependable. Switching to a different format like JSON5 would break all that. For example JSON5 doesn’t work with MySQL, Postgres, Mongo or other storage engines with JSON support. I think it’s not a good scenario.
JSON is like the lowest common denominator of serialization formats, even though much better ones exist. But that is actually one of the reasons it’s successful - JSON is understood pretty much everywhere.

JSON and the other formats are standardized, so switching implementations will not change the interpretation of the data. Switching the format is a huge breaking change.
Using a different serialization format like JSON5 or EDN as a default would be fine, too, but not by switching out the interface implementation, but rather by using a different concrete class and having the code depend on that, e.g. EdnSerializer or Jsin5Serializer.
The data serialization format is important and needs to be dependable.

[ihor-sviziev]

I would like to add an ability to use it as viewModel in phtml files by extending \Magento\Framework\View\Element\Block\ArgumentInterface. What do you think?

[fooman]

I am with @Vinai and I think this discussion might be a good starting point for broadening it.

The current Magento approach is to avoid adding to existing interfaces as this would break other implementations of said interface.

Instead we are introducing a new interface and deprecate the old (it's really just replacing one way of breaking with a different one).

/** @deprecated */
interface ExistingInterface
{
    public methodOne();
}

interface NewInterface
{
    public methodOne();
    public MethodTwo();
}

class DefaultClass implements NewInterface
{
}

On balance I believe this is optimising for the wrong audience. I am convinced that there are more users of ExistingInterface than there are alternative implementations of DefaultClass that do not look like this:

class ReplacedClass extends DefaultClass
{
}

I think we should take a look at the polices and approach so that we can end up with:

interface ExistingInterface
{
    public methodOne();
    public MethodTwo();
}

as that would create less work over all.

[lenaorobei]

@Vinai @fooman thank you for the feedback, your arguments were compelling enough and I updated the proposal.

Now the proposal states:

• Deprecate the Magento\Framework\Serialize\SerializerInterface and add annotation @see use Magento\Framework\Serialize\Serializer\Json instead.
• Add two new methods to the Magento\Framework\Serialize\Serializer\Json. Implementation will use low-level PHP json_encode/json_decode under the hood and will throw InvalidArgumentException if encoding/decoding cannot be performed.

[Vinai]

Thanks for listening!