Remove zend json from framework json classes

[dmanners]

Description

I updated the encoder and decoder classes to use the new serializer class for the case that someone other class is using these directly.
I updated the json helper to stop using the encoder and decoder but instead going directly to the serializer class with half an eye on the removal of the encoder and decoder I thought this was a good option compared to simply updating the encoder and decoder classes.
Fix the import customer composite data test to work with the new serializer.

I was tempted to go through and replace the usage of these classes but I thought that can wait as that is a much bigger change.

I was a bit worried that this change only broke 1 other test class, but still.

Fixed Issues (if relevant)

1. Upgrade ZF components. Zend_Json #9236: Upgrade ZF components. Zend_Json

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[ishakhsuvarov]

Looks like these parameters may be deprecated now.

[okorshenko]

Hi @dmanners Thank you for this PR.
Unfortunately this PR brakes a lot of logic since it can not process DataObjects:
For example: \Magento\Catalog\Block\Adminhtml\Product\Composite\Update\Result::_toHtml
As a result, we have lots of broken functional tests:
Magento\Sales\Test\TestCase\MoveRecentlyComparedProductsOnOrderPageTest,
Magento\Wishlist\Test\TestCase\ConfigureProductInCustomerWishlistOnBackendTest and some other

We need to adjust the code and cover this cases.
Thank you

[dmanners]

Hi @okorshenko and @ishakhsuvarov thanks for your time on this review.

I would be interested as to why the travis build was showing green even if the tests failed/did not work as expected or does it not run the functional test suite?

Anyway my thoughts on the code change needed would be either.

1. Add an if ($data instance of \Magento\Framework\DataObject) { $data = $data->toArray(); } inside the Json serializer class.
2. Update the places that call this class to perform the toArray before passing the data to the serializer class.

I am leaning towards the 2nd option here since the type hint for the serialize method is @param string|int|float|bool|array|null $data and as such should not "work" with the DataObject.

What are your thoughts?

[ishakhsuvarov]

@dmanners

I would be interested as to why the travis build was showing green even if the tests failed/did not work as expected or does it not run the functional test suite?

Functional tests suite which is executed on Travis CI has limited number of tests, which covers only the most critical scenarios. Unfortunately running the whole functional tests suite is currently possible only with Magento's internal CI.

Add an if ($data instance of \Magento\Framework\DataObject) { $data = $data->toArray(); } inside the Json serializer class.

I would suggest looking at this approach to be implemented in the Encoder/Decoder classes, since that would minimize the risk of breaking existing third-party extensions which may potentially rely on that behavior.

Update the places that call this class to perform the toArray before passing the data to the serializer class.

This the classes in question are already deprecated I would only look into possibility to eliminate their usages, but not to fix those in some ways.

[ishakhsuvarov]

Hey @dmanners
Are there any updates regarding this PR? Would you like any assistance on this? Please reach us in the comments or directly if that is the case.
Thanks!

[dmanners]

hi @ishakhsuvarov, nothing to update here as yet. Will hope to get sometime on this coming up.

One thing that is a concern/problem is that Zend_Json works with "simple objects" but the lib version does not. For example the test at

magento2/lib/internal/Magento/Framework/Serialize/Test/Unit/Serializer/JsonTest.php

Line 48 in 4c9194d

[$dataObject, '{}'],

highlights that the lib should not "work" with the DataObject but there are many places in the code that work with this object and expect serialization to "work".

[ishakhsuvarov]

@dmanners I am closing this for now due to inactivity. Please reopen if you wish to continue at any time.