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 the unit test in the CustomerImportExport module #8706

Merged

Conversation

dmanners
Copy link
Contributor

@dmanners dmanners commented Feb 28, 2017

Since Zend Framework1 is EOF then we should start to move away from it.

This PR removes the usage of Zend_Json in the CustomerImportExport module.

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

Cool! 👍

But, it can be removed from all tests at once, I observe 38 occurrences of Zend_Json in *Test.php.

Just to decrease the overhead of multiple pull requests processing ;)

@dmanners
Copy link
Contributor Author

Hi @orlangur I have been working through module by module simply so that I can encapsulate the changes. I dont just want to do a find an replace on the full code base and then try to fix all the problems. For me smaller changes like this is actually how you get a monolith moving in the right direction.

@orlangur
Copy link
Contributor

@dmanners, ok, you can go your way, no offense.

just want to do a find an replace on the full code base and then try to fix all the problems

That's exactly how I like to do it :) Most likely there would be no problem with tests, otherwise some of them could be left untouched (like this: orlangur@77a6b51). Fix as much problems as possible all over the code + leave some tricky cases for further atomic implementation. The thing which would make me sad is fixing tests manually which can be done automatically and only once.

@dmanners
Copy link
Contributor Author

@orlangur dont worry no offence taken. I just started this by stripping out Zend_Json overall places and then had to scale it back step by step. These things take time.

@vrann vrann self-assigned this Feb 28, 2017
@vrann vrann self-requested a review February 28, 2017 13:22
@vrann vrann added this to the February 2017 milestone Feb 28, 2017
@maksek maksek modified the milestones: February 2017, March 2017 Mar 1, 2017
@okorshenko okorshenko added this to the March 2017 milestone Mar 1, 2017
@magento-team magento-team merged commit c630131 into magento:develop Mar 8, 2017
@ishakhsuvarov
Copy link
Contributor

@dmanners We have now merged your PR to the develop branch. Thank you for your contribution!

@dmanners dmanners deleted the remove-zend-json-customerimportexport 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.

7 participants