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.

Sign up for GitHub

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

Jump to bottom

Remove zend json from package info #10340

Merged
merged 2 commits into from
Jul 27, 2017
Merged

Remove zend json from package info #10340

merged 2 commits into from
Jul 27, 2017

Conversation

dmanners
Copy link
Contributor

@dmanners dmanners commented Jul 26, 2017 •
edited by okorshenko
Loading

Description

Replace Zend_Json usage in Framework/Module/PackageInfo.php …

  • inject the new \Magento\Framework\Serialize\Serializer\Json class,
  • update the load method to use the Serializer,
  • update the load method to catch the InvalidArgumentException exception

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 26, 2017 19:51
@dmanners
Replace Zend_Json usage in Framework/Module/PackageInfo.php
f706fdd 
 - inject the new \Magento\Framework\Serialize\Serializer\Json class,
 - update the load method to use the Serializer,
 - update the load method to catch the InvalidArgumentException exception
@dmanners
Update constructor docblock
fd155b3
dmanners
dmanners commented Jul 26, 2017
View reviewed changes
lib/internal/Magento/Framework/Module/PackageInfo.php
} catch (\Zend_Json_Exception $e) {
throw new \Zend_Json_Exception(
$packageData = $this->serializer->unserialize($jsonData[$key]);
} catch (\InvalidArgumentException $e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of catching this and then throwing the same exception. I was thinking about adding a new exception for this case here. What are people's thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean specific exception for Json or Serialization?
I assume this specific fix is fine, since there is no major change.

@okorshenko okorshenko self-assigned this Jul 26, 2017
@okorshenko okorshenko added this to the July 2017 milestone Jul 26, 2017
@magento-team magento-team merged commit fd155b3 into magento:develop Jul 27, 2017
magento-team pushed a commit that referenced this pull request Jul 27, 2017
@ishakhsuvarov
MAGETWO-71060: Remove zend json from package info #10340
44b2adf
magento-team pushed a commit that referenced this pull request Jul 27, 2017
@ishakhsuvarov
Merge pull request #1365 from magento-engcom/develop-prs
27db72c 
[EngCom] Public Pull Requests
 - MAGETWO-71060 Remove zend json from package info #10340
 - MAGETWO-71059 Remove Zend_Json from setup #10339
@dmanners dmanners deleted the remove-zend-json-from-package-info branch July 28, 2017 07:00
magento-team pushed a commit that referenced this pull request Oct 3, 2017
@irenelagno
MAGETWO-80185: [2.2.x] - Remove zend json from package info #10340
4971b07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishakhsuvarov ishakhsuvarov ishakhsuvarov left review comments

Assignees

@okorshenko okorshenko

Labels
Progress: accept
Projects
None yet
Milestone
July 2017
Development

Successfully merging this pull request may close these issues.
4 participants
@dmanners @ishakhsuvarov @magento-team @okorshenko