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 theme #9262

Merged
merged 4 commits into from
Apr 19, 2017

Conversation

dmanners
Copy link
Contributor

@dmanners dmanners commented Apr 15, 2017

Description

Since Zend Framework1 is EOL we should replace the usages. In this PR I replace the usage of Zend_Json in Magento Theme with the new serializer class. Part of the #9236 task.

Fixed Issues (if relevant)

  1. Upgrade ZF components. Zend_Json #9236
  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)

@adragus-inviqa
Copy link
Contributor

All this looks ok, but I was just wondering: seeing how the M2 JSON serializer is only a dumb wrapper over json decode/encode, how would we handle encoding/decoding errors? We'd have to use json_last_error() now? At least Zend_Json had exceptions...

I'm saying this, because this PR removes a try/catch block that would've been otherwise useful. And said try/catch block was around a cookie read no less. Aka user input. You should never trust that. Imagine json_decode($_GET['param']) and not checking for errors. Not cool.

@adragus-inviqa
Copy link
Contributor

adragus-inviqa commented Apr 17, 2017

Well, yes, that's what I mean. But that also means that the behaviour of the serializer must change, breaking compatibility, because it doesn't throw at the moment.

And since it's a BC break, they won't allow it. Your best option is a new SerializeWithThrow decorator or something, wrapping the original one. That should also be a new PR, btw.


Or maybe we can add a new method to the existing Serializer called getErrors() or something? This way, we won't be breaking BC. I'm not sure about this way of doing things, as people might forget to check the errors, but it's an alternative.

@dmanners
Copy link
Contributor Author

Yeah, I think having another class specifically to deal with json throwing an exception is a bit of overkill. I will put together a PR for the serializer with the exception handling and see what the core Magento team say.

@okorshenko
Copy link
Contributor

@dmanners @adragus-inviqa thank you for raising the issue with Exceptions. This is known issue for us and Magento Core Team is looking for proper solution right now.

@magento-team magento-team merged commit 4ac9508 into magento:develop Apr 19, 2017
magento-team pushed a commit that referenced this pull request Apr 19, 2017
magento-team pushed a commit that referenced this pull request Apr 19, 2017
@magento-team
Copy link
Contributor

@dmanners Thank you for the contribution

@dmanners dmanners deleted the remove-zend-json-from-theme branch July 26, 2017 12:00
magento-devops-reposync-svc pushed a commit that referenced this pull request Sep 10, 2024
…est-pr-exclude

[Bengals] ACQE-7010: Add mftf third party test in pr exclude
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.

4 participants