Skip to content

Remove zend json from data object #10306

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

Merged

Conversation

dmanners
Copy link
Contributor

@dmanners dmanners commented Jul 24, 2017

Description

The DataObject class has a method toJson that uses Zend_Json. Since this at EOF we should replace the usage. Unfortunately in this case that is not so "simple" after some discussion with @okorshenko it was decided to create a new static class for this single case. This should not be used in other places but for this case it is "ok".

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)

dmanners added 2 commits July 24, 2017 07:32
…nConverter.

 - This is only in place for this one use case and should not be used for other times with converting json
@okorshenko okorshenko self-assigned this Jul 24, 2017
@okorshenko okorshenko added this to the July 2017 milestone Jul 24, 2017
@ihor-sviziev
Copy link
Contributor

@dmanners Why do we need new object with static method? We could add to constructor optional parameter (with fallback to object manager)

@dmanners
Copy link
Contributor Author

@ihor-sviziev I am more than happy to find another option but unfortunately going the route of adding an optional dependency causes big breaks in the test suite. I am not sure exactly how the object manager changes in the test suite so there could be an option to mock the fallback during the test suite.

@orlangur
Copy link
Contributor

Why do we need to add new behavior with @throws \InvalidArgumentException instead of just calling json_encode?

What do you think about deprecating toJson method also to decrease amount of DataObject class responsibilities?

@okorshenko
Copy link
Contributor

Hi @orlangur removing the method from DataObject interface will be BiC that we would like to avoid. The standard approach for adding new dependencies to DataObject does not work well due to modularity. So we suggested the way that @dmanners implemented.

@dmanners
Copy link
Contributor Author

@orlangur normally I would like to keep away from the direct calls to json_encode but in the case of this class with the combination of the deprecation of the toJson I think I could live with myself. This class itself needs a lot of love or at least the usages of this class need some love and care so highlighting that is not a bad thing in my books. What do you think @okorshenko

@orlangur
Copy link
Contributor

@okorshenko I did not say a word regarding method removal, just deprecation. As to me static class introduction with comment to use it in only one place is much uglier than simply using json_encode in this place (the only difference compared to serializer https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Serialize/Serializer/Json.php#L24 is not throwing an exception which corresponds to old method behavior).

@korostii
Copy link
Contributor

Hi @okorshenko
As @orlangur says, there's nothing wrong with both keeping the method present and avoiding new dependencies.
However, what's the point of using \Magento\Framework\Serialize\JsonConverter instead?

After all, the new @throws declaration a BiC change by itself.
How is it any better than using json_encode directly?

@dmanners dmanners deleted the remove-zend-json-from-data-object branch July 26, 2017 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants