Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove zend json from json controller #10342

Merged

Conversation

@dmanners
Copy link
Contributor

commented Jul 26, 2017

Description

Removing the usage of Zend_Json in the Json Controller. Deprecating the method setData to encourage the use of the more type specific methods in this class after talking through options with @okorshenko

Fixed Issues (if relevant)

  1. #9236: Upgrade ZF components. Zend_Json

Manual testing scenarios

  1. ...
  2. ...

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)
dmanners added 6 commits Jul 17, 2017
Remove the usage of Zend_Json from the controller results json file.
 - use the \Magento\Framework\Serialize\Serializer\Json instead of Zend_Json
Check to see if we are dealing with an object with a toJson method
 - if we are dealing with something like new \Magento\Framework\DataObject() being used as a response object
Update Controller/Result/Json setData
 - deprecate this method to encourage usage of other methods in this class,
 - be stricter on types passed into this method,
@dmanners

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2017

This PR I expect to come up with plenty of talk/opinions. I am happy to hear what people think would be other options if there are some.

@okorshenko okorshenko self-assigned this Jul 26, 2017

@okorshenko okorshenko added this to the July 2017 milestone Jul 26, 2017

@okorshenko okorshenko added the develop label Jul 26, 2017

*/
public function __construct(
InlineInterface $translateInline,
\Magento\Framework\Serialize\Serializer\Json $serializer = null

This comment has been minimized.

Copy link
@orlangur

orlangur Jul 26, 2017

Contributor

Why do we rely on particular implementation and not interface by the way?
https://github.com/magento/magento2/blob/develop/app/etc/di.xml#L162

This comment has been minimized.

Copy link
@dmanners

dmanners Jul 26, 2017

Author Contributor

#8331 (comment) was the original comment from the team on the matter,

This comment has been minimized.

Copy link
@orlangur

orlangur Jul 26, 2017

Contributor

Thanks, I expected something like this. Not sure why such interface was introduced at all if we keep creating json-only code.

Needs to be mentioned somewhere to avoid confusion like #10335 (comment)

@magento-team magento-team merged commit 41e118b into magento:develop Jul 27, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@dmanners dmanners deleted the dmanners:remove-zend-json-from-json-controller branch Jul 28, 2017

@okorshenko

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

@dmanners This pull request has been reverted due to broken functionality. We need to review this change one more time. Thank you

@dmanners dmanners restored the dmanners:remove-zend-json-from-json-controller branch Jul 31, 2017

@dmanners dmanners deleted the dmanners:remove-zend-json-from-json-controller branch Jul 31, 2017

@dmanners

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

@okorshenko sure. Do you have a specific use/test case that this broke for so that I can have something to review when thinking about possible options here.

Thanks

* @throws \InvalidArgumentException
* @throws \Magento\Framework\Exception\LocalizedException
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
* @deprecated

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

I disagree with deprecating this method. setData is for setting data which will be encoded and setJsonData is for setting raw data.

Two arguments which has no effect should be deprecated though.

Please change method description accordingly.

*/
public function setData($data, $cycleCheck = false, $options = [])
{
$this->json = \Zend_Json::encode($data, $cycleCheck, $options);

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

This method is just setter, no need to call another setter, Magento is already slow enough.

*/
public function setData($data, $cycleCheck = false, $options = [])
{
$this->json = \Zend_Json::encode($data, $cycleCheck, $options);
if ($data instanceof \Magento\Framework\DataObject) {

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

Why need this explicitly?

This comment has been minimized.

Copy link
@ishakhsuvarov

ishakhsuvarov Aug 2, 2017

Contributor

This is needed to maintain compatibility with the old implementation which took care of this inside.

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

I see...

public static function encode($valueToEncode, $cycleCheck = false, $options = array())
    {
        if (is_object($valueToEncode)) {
            if (method_exists($valueToEncode, 'toJson')) {
                return $valueToEncode->toJson();
            } elseif (method_exists($valueToEncode, 'toArray')) {
                return self::encode($valueToEncode->toArray(), $cycleCheck, $options);
            }
        }

Why it is not maintained for any object then?

return $this->setArrayData($data->toArray());
}
if (is_array($data)) {

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

Please remove this if.

This comment has been minimized.

Copy link
@ishakhsuvarov

ishakhsuvarov Aug 2, 2017

Contributor

Please clarify

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

setData should be as old one, thus setData([]) just encodes array with no necessity for additional logic.

return $this->setArrayData($data);
}
if (is_string($data)) {

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

Passing string parameter did encode $data before, please remove this if.

This comment has been minimized.

Copy link
@ishakhsuvarov

ishakhsuvarov Aug 2, 2017

Contributor

Please clarify

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

setData('someString') was setting json_encode('someString') and not just 'someString'.

* @return $this
* @throws \InvalidArgumentException
*/
public function setArrayData(array $data)

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

Please remove this method as it does not bring any value.

This comment has been minimized.

Copy link
@ishakhsuvarov

ishakhsuvarov Aug 2, 2017

Contributor

This method was introduced to deprecate and remove setData method, which has too much responsibilities.

This comment has been minimized.

Copy link
@orlangur

orlangur Aug 2, 2017

Contributor

This is what I strongly disagree with. Its responsibility is quite clear - encode value and set it. There is no need to think which value you're passing.

setData - pass value and it will be encoded
setJsonData - pass string and it will be set as is

I do agree that setData($object) could be discouraged and corresponding if could be marked as deprecated for further removal and setData method simplification. But introduction of setArrayData method looks like a mess to me.

@orlangur

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

@dmanners I didn't do code review previous time but now did, please check my suggestions and prepare a new PR (merged PR cannot be reopened AFAIK).

@dmanners

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

@orlangur thanks for the feedback, I appreciate the thoughts. I need to re-think these changes anyway so will take your ideas on board and see what I can put together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.