Remove zend json from customer data #10259

dmanners · 2017-07-17T10:17:11Z

Description

As both Zend_Json and \Magento\Framework\Json\EncoderInterface are now deprecated I have migrated the usage across the the accepted \Magento\Framework\Serialize\Serializer\Json class.

Fixed Issues (if relevant)

Upgrade ZF components. Zend_Json #9236: Upgrade ZF components. Zend_Json

Manual testing scenarios

...
...

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)

- remove zend call and use the Magento Framework, - move work from template into app/code/Magento/Customer/Block/CustomerScopeData.php

…terface in the CustomerScopeData block, - Use \Magento\Framework\Serialize\Serializer\Json instead when dealing with serilization - Update test to mirror the change in the main class

dmanners · 2017-07-17T10:25:53Z

Not 100% sure if this is the "best way" ™️ for replacing the deprecated class that is being injected in. Happy for feedback and change requests.

ihor-sviziev · 2017-07-17T10:53:02Z

@dmanners I see that app/code/Magento/Customer/Block/CustomerScopeData.php file didn't exists in 2.1 releases, so I think it's ok that we can change file before release.

dmanners · 2017-07-17T10:54:03Z

Ah @ihor-sviziev thanks for the information. I had not noticed it before.

okorshenko · 2017-07-17T22:03:36Z

Hi @dmanners
Unfortunately, we can not add new methods to existing @api classes and change constructor signatures. develop branch is designed for 2.3 but this class was added in 2.2. so this will be BiC

dmanners · 2017-07-18T06:57:13Z

Hi @okorshenko, thanks for the feedback. I have a few follow up questions then.

If this is a new class in 2.2 can I simply pr against 2.2.0-preview or is it too later to get changes into that branch?
Is there a plan/blog post out there on the "perfect ™️" approach for changing these sort of classes? This might be a simple example but I am sure this is not the first and wont be the last time someone wants to make a change to an @api tagged class.

Happy to here any options for making this change.

orlangur · 2017-07-18T11:58:08Z

app/code/Magento/Customer/Block/CustomerScopeData.php

+     */
+    public function getSerializedInvalidationRules()
+    {
+        return $this->serializer->serialize(


I don't think serialization in block is a good idea. It is better to just return raw data and JSON (or whatever else) encode in template.

Due to such get...Json methods in Magento 1 some extensions had to decode string and then re-encode just to modify array.

Thanks for the feedback, generally I agree with you here. I considered putting in one function to cover the serialization of the data. And one to cover the getting of the data. This would then allow 3rd party extensions to manipulate the data. Currently with the data stuck in the template it could not be changed easily anyway. I will have a look around for other examples of code like this.

orlangur · 2017-07-18T15:32:04Z

So all blocks should be @api annotated. This test checks that all blocks declared in layout files are public

Hm, this is something new I was not aware of...

@okorshenko as all blocks became @api annotated are you sure that we cannot add new public methods to them? Unlike adding some methods to interface it sounds a bit weird to me.

@dmanners,

approach for changing these sort of classes?

http://devdocs.magento.com/guides/v2.1/contributor-guide/backward-compatible-development/index.html more or less describes approach, if some cases are not covered yet we can extend it with time.

…C rules - keep the $jsonEncoder injection but remove the type, - mark the constructor as PHPMD.UnusedFormalParameter, - split out the config and the serilization to happen in two methods,

orlangur · 2017-07-18T16:46:23Z

app/code/Magento/Customer/Block/CustomerScopeData.php

+     */
+    public function getSerializedInvalidationRules()
+    {
+        return $this->serializer->serialize($this->getInvalidationRules());


Still... What are the advantages compared to echo json_encode($block->getInvalidationRules(); in template? As to me encoding is purely responsibility of template.

Another thing which just came to my mind: do we have a recommendation to leave JS component declaration in templates? Thus they may be overridden more easily, without touching PHP classes. At least I was not able to find similar occurrence of /js/ or '*' in *.php.

Not sure about the leaving of the js declaration in the templates. Though to me this feels like config and to have to change a full template to change the config feels odd.

With regards to the json_encode in the template I dont agree here. I feel that we should either be using json_encode everywhere or wrap it's usage across the system, which would include templates. This way if a mass change is needed it is easier to manage and roll out.

Have no idea which kind of mass change could it be :D

Ok, I see your point, you want to encapsulate serializer logic in block class and don't make template aware of it (while it does escapeHtml and other similar things). There is nothing to discuss here, let's wait for the feedback from Oleksii and then maybe put a common recommendation for such situation somewhere to keep the code consistent.

Yeah something like escapeHtml would be a nice option here also thought about that. It's above this PR but I think some "common" way to do this through templates would be amazing.

okorshenko · 2017-07-18T18:21:49Z

Hi @dmanners and @orlangur
Sorry for confusing.
We can not add public methods to @api classes in patch releases, but we can do that in minor. So 2.3 is a minor release and we can add this method. The issue was with constructor
Here is SVC report

MAJOR - Magento\Customer\Block\CustomerScopeData::__construct - [public] Method parameter changed
MINOR - Magento\Customer\Block\CustomerScopeData::getSerializedInvalidationRules - [public] Method has been added

So the issue was with the constructor.

ihor-sviziev · 2017-07-18T18:45:01Z

app/code/Magento/Customer/Block/CustomerScopeData.php

     */
    public function __construct(
        \Magento\Framework\View\Element\Template\Context $context,
-        \Magento\Framework\Json\EncoderInterface $jsonEncoder,
-        array $data = []
+        $jsonEncoder,


Looks like you removed interface by mistake.

yup, will add that back in.

…erScopeData

- moved component configuration to the template file

- rework fix for 2.2.1 without minor change

Ctucker9233 · 2019-03-16T19:00:18Z

@magento-team What version of 2.2 will have this included? As of 2.2.7 it doesn't appear to be in there.

orlangur · 2019-03-17T22:18:35Z

@Ctucker9233 you can check corresponding 2.2 commits: http://prntscr.com/mz7vk2

dmanners added 2 commits July 17, 2017 10:03

Remove Zend_Json call from js/customer-data/invalidation-rules.phtml

2e72fb0

- remove zend call and use the Magento Framework, - move work from template into app/code/Magento/Customer/Block/CustomerScopeData.php

Replace the usage of the depricated \Magento\Framework\Json\EncoderIn…

0a7ee59

…terface in the CustomerScopeData block, - Use \Magento\Framework\Serialize\Serializer\Json instead when dealing with serilization - Update test to mirror the change in the main class

okorshenko self-assigned this Jul 17, 2017

okorshenko added this to the July 2017 milestone Jul 17, 2017

okorshenko added the develop label Jul 17, 2017

magento-team added Progress: accept develop labels Jul 17, 2017

okorshenko removed the Progress: accept label Jul 17, 2017

okorshenko added the Progress: needs update label Jul 17, 2017

orlangur reviewed Jul 18, 2017

View reviewed changes

Update the CustomerScopeData changes to use serializer to fit with Bi…

eaad78c

…C rules - keep the $jsonEncoder injection but remove the type, - mark the constructor as PHPMD.UnusedFormalParameter, - split out the config and the serilization to happen in two methods,

orlangur reviewed Jul 18, 2017

View reviewed changes

okorshenko removed the Progress: needs update label Jul 18, 2017

ihor-sviziev reviewed Jul 18, 2017

View reviewed changes

dmanners added 2 commits July 18, 2017 19:11

Add back in the type for the $jsonEncoder object injected into Custom…

5568b8d

…erScopeData

Add back in the mock of the jsonEncoder to the test case

31764a5

magento-team added Progress: accept develop labels Jul 19, 2017

magento-team merged commit 31764a5 into magento:develop Jul 19, 2017

magento-team pushed a commit that referenced this pull request Jul 19, 2017

MAGETWO-70775: Remove zend json from customer data #10259

7a4e077

- moved component configuration to the template file

magento-team pushed a commit that referenced this pull request Jul 19, 2017

MAGETWO-70775: Remove zend json from customer data #10259

d83e0e9

dmanners deleted the remove-zend-json-from-customer-data branch July 26, 2017 12:00

magento-team pushed a commit that referenced this pull request Oct 3, 2017

MAGETWO-80177: [2.2.x] - Remove zend json from customer data #10259

ad118e6

magento-team pushed a commit that referenced this pull request Oct 3, 2017

MAGETWO-80177: [2.2.x] - Remove zend json from customer data #10259

4215a8b

- rework fix for 2.2.1 without minor change

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove zend json from customer data #10259

Remove zend json from customer data #10259

dmanners commented Jul 17, 2017 •

edited by okorshenko

Loading

dmanners commented Jul 17, 2017

ihor-sviziev commented Jul 17, 2017

dmanners commented Jul 17, 2017

okorshenko commented Jul 17, 2017

dmanners commented Jul 18, 2017

orlangur Jul 18, 2017

dmanners Jul 18, 2017

orlangur commented Jul 18, 2017

orlangur Jul 18, 2017

dmanners Jul 18, 2017

orlangur Jul 19, 2017

dmanners Jul 19, 2017

okorshenko commented Jul 18, 2017

ihor-sviziev Jul 18, 2017

dmanners Jul 18, 2017

Ctucker9233 commented Mar 16, 2019

orlangur commented Mar 17, 2019

Remove zend json from customer data #10259

Remove zend json from customer data #10259

Conversation

dmanners commented Jul 17, 2017 • edited by okorshenko Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

dmanners commented Jul 17, 2017

ihor-sviziev commented Jul 17, 2017

dmanners commented Jul 17, 2017

okorshenko commented Jul 17, 2017

dmanners commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okorshenko commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ctucker9233 commented Mar 16, 2019

orlangur commented Mar 17, 2019

dmanners commented Jul 17, 2017 •

edited by okorshenko

Loading