Remove Zend1 Json from Magento Captcha module #8331

dmanners · 2017-01-30T13:37:25Z

Zend Framework 1 has been at end of life since middle of 2016. Zend Framework 2 comes with a json module that works in the same way as the Zend Framework 1 version.

In this pull request I have updated the captcha module to use the zend framework 2 versions of Zend_Json. I have also updated the code to use json decode via object since that is the new default in Zend Framework 2.

ihor-sviziev · 2017-01-30T16:14:09Z

Magento has own lib for json encode/decode. I think it should be used instead in all places:
https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Json/EncoderInterface.php
Also classes of Magento\Framework\Json should be fixed to use new version of zend json

dmanners · 2017-01-30T16:16:32Z

@ihor-sviziev In an ideal world that is the approach that I wanted to take and update the lib to use Zend2 rather than Zend1. The honest reason that I did not do this is because updating the main lib and then all the 100s of places that are using it would be far to big a change set.

I also could not get the test suit to run to cover these changes.

dmanners · 2017-01-30T21:00:50Z

I see that there are 2 ways to approach this either.

Migrate all usage of Zend_Json to use the Magento lib version. Then once this is done migrate the lib version to Zend2.
Just migrate one module at a time directly to Zend2,

I can see benefits to both approaches and do not really mind which way to do. I would be happy to work through some of the Zend_Json calls either migrating to the lib call or direct calls to Zend2.

buskamuza

Please use https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Serialize/Serializer/Json.php

https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Json/EncoderInterface.php may be deprecated and removed in future as it has unnecessary responsibilities.

dmanners · 2017-01-31T10:19:03Z

@buskamuza I have updated the module to now use Magento/Framework/Serialize/Serializer/Json.php when dealing with Json.

dmanners · 2017-02-02T10:54:55Z

All other pull requests in the "Remove Zend1 from Magento Captcha" series can be found:

ishakhsuvarov · 2017-02-03T16:04:00Z

app/code/Magento/Captcha/Controller/Refresh/Index.php

+    public function __construct(
+        Context $context,
+        \Magento\Captcha\Helper\Data $captchaHelper,
+        \Magento\Framework\Serialize\SerializerInterface $serializer


Please inject this dependency using optional parameter.

ishakhsuvarov · 2017-02-03T16:05:22Z

app/code/Magento/Captcha/Model/Customer/Plugin/AjaxLogin.php

     * @param array $formIds
     */
    public function __construct(
        CaptchaHelper $helper,
        SessionManagerInterface $sessionManager,
        JsonFactory $resultJsonFactory,
+        \Magento\Framework\Serialize\SerializerInterface $serializer,


Please inject this dependency using optional parameter.

ishakhsuvarov

Please improve constructor injections, so that new dependencies are optional parameters.

dmanners · 2017-02-03T23:43:11Z

@ishakhsuvarov should all be covered now

ishakhsuvarov · 2017-02-06T14:28:05Z

Hi @dmanners
We still have some discussion regarding json encoding internally. I am going to proceed with your PR as soon as it is possible.
Sorry for the delay.

dmanners · 2017-02-06T14:29:52Z

Hi @ishakhsuvarov no rush. I want to get the "best" option here.

ishakhsuvarov · 2017-02-07T14:53:17Z

Hey @dmanners
Looks like every serialization in the change set requires the data to be explicitly encoded in JSON. Sadly, SerializeInterface can not guarantee, that preference for it would always be Magento\Framework\Serialize\Serializer\Json in future, so, the best solution for now would be to use not interface but Json Serializer class.

ishakhsuvarov · 2017-02-07T14:48:34Z

app/code/Magento/Captcha/Controller/Adminhtml/Refresh/Refresh.php

+        \Magento\Framework\Serialize\SerializerInterface $serializer = null
+    ) {
+        parent::__construct($context);
+        $this->serializer = $serializer ?: \Magento\Framework\App\ObjectManager::getInstance()


Please use Magento\Framework\Serialize\Serializer\Json instead of SerializerInterface, since we can not guarantee that preference will remain Json in the future.

Since you are not modifying the existing constructor but adding a new one – you can inject the dependency in a normal way.

@ishakhsuvarov for the second point do you mean I do not need to default to null?

ishakhsuvarov · 2017-02-07T14:49:03Z

app/code/Magento/Captcha/Controller/Refresh/Index.php

        $this->captchaHelper = $captchaHelper;
+        $this->serializer = $serializer ?: \Magento\Framework\App\ObjectManager::getInstance()


Please use Magento\Framework\Serialize\Serializer\Json instead of SerializerInterface, since we can not guarantee that preference will remain Json in the future.

ishakhsuvarov · 2017-02-07T14:49:50Z

app/code/Magento/Captcha/Model/Customer/Plugin/AjaxLogin.php

    ) {
        $this->helper = $helper;
        $this->sessionManager = $sessionManager;
        $this->resultJsonFactory = $resultJsonFactory;
+        $this->serializer = $serializer ?: \Magento\Framework\App\ObjectManager::getInstance()


Please use Magento\Framework\Serialize\Serializer\Json instead of SerializerInterface, since we can not guarantee that preference will remain Json in the future.

ishakhsuvarov · 2017-02-07T14:51:41Z

app/code/Magento/Captcha/Test/Unit/Controller/Refresh/IndexTest.php

@@ -62,24 +67,37 @@ protected function setUp()
        $this->viewMock = $this->getMock(\Magento\Framework\App\ViewInterface::class);
        $this->layoutMock = $this->getMock(\Magento\Framework\View\LayoutInterface::class);
        $this->flagMock = $this->getMock(\Magento\Framework\App\ActionFlag::class, [], [], '', false);
+        $this->serializerMock = $this->getMock(
+            \Magento\Framework\Serialize\SerializerInterface::class,


Please use Magento\Framework\Serialize\Serializer\Json instead of SerializerInterface.

ishakhsuvarov · 2017-02-07T14:52:05Z

app/code/Magento/Captcha/Test/Unit/Model/Customer/Plugin/AjaxLoginTest.php

@@ -79,31 +84,41 @@ protected function setUp()
        $this->captchaHelperMock->expects($this->once())->method('getCaptcha')
            ->with('user_login')->will($this->returnValue($this->captchaMock));
        $this->formIds = ['user_login'];
+        $this->serializerMock = $this->getMock(
+            \Magento\Framework\Serialize\SerializerInterface::class,


Please use Magento\Framework\Serialize\Serializer\Json instead of SerializerInterface.

…work\Serialize\SerializerInterface

…er/Refresh/Index.php to use the lib json serializer object

…to/Captcha/Controller/Adminhtml/Refresh/Refresh.php

…tcha helper

…on the interface that could change

okorshenko · 2017-02-09T19:42:22Z

@dmanners that you for this PR. you made module captcha better again!

antonkril requested a review from buskamuza January 30, 2017 17:16

dmanners force-pushed the zend1-json-removed branch from 62ddff2 to b2bc36f Compare January 30, 2017 21:04

buskamuza suggested changes Jan 30, 2017

View reviewed changes

dmanners force-pushed the zend1-json-removed branch from b2bc36f to 92c317a Compare January 31, 2017 07:58

dmanners force-pushed the zend1-json-removed branch 2 times, most recently from 8dae3db to 311a7be Compare February 2, 2017 08:25

dmanners mentioned this pull request Feb 2, 2017

Remove the usage of Zend_Db_Expr from the captcha module #8389

Closed

dmanners mentioned this pull request Feb 2, 2017

Remove Zend1 captcha from Magento2 Captcha module #8356

Merged

dmanners force-pushed the zend1-json-removed branch from 8f09908 to 65de9c6 Compare February 3, 2017 12:26

ishakhsuvarov self-assigned this Feb 3, 2017

ishakhsuvarov added the improvement label Feb 3, 2017

ishakhsuvarov requested changes Feb 3, 2017

View reviewed changes

ishakhsuvarov requested changes Feb 7, 2017

View reviewed changes

dmanners added 7 commits February 7, 2017 16:37

Replace usage of Zend_Json in Magento Captcha with the \Magento\Frame…

d3fa692

…work\Serialize\SerializerInterface

Replace the usage of json_encode in app/code/Magento/Captcha/Controll…

5d68d6c

…er/Refresh/Index.php to use the lib json serializer object

Replace the usage of json_encode and object manager in app/code/Magen…

65c9eff

…to/Captcha/Controller/Adminhtml/Refresh/Refresh.php

Update the phpdocs in the Captcha admin controller to include the cap…

e80321a

…tcha helper

Update phpcods to include the serializer object

debea18

Fix serializer type hint in the admin controller of mage captcha module

a26f8da

Make sure that the new dependancy on SerializerInterface is not required

298d3c5

dmanners added 2 commits February 7, 2017 16:37

Make sure that the optional dependancies are last in the constructor

6a2e353

Specifically ask for the Json Serializer object and not just relying …

c5bb856

…on the interface that could change

dmanners force-pushed the zend1-json-removed branch from e916d93 to c5bb856 Compare February 7, 2017 16:44

ishakhsuvarov approved these changes Feb 8, 2017

View reviewed changes

ishakhsuvarov added the Progress: accept label Feb 8, 2017

mmansoor-magento merged commit c5bb856 into magento:develop Feb 9, 2017

okorshenko added this to the February 2017 milestone Feb 11, 2017

vrann added the Award: advanced label Mar 11, 2017

dmanners deleted the zend1-json-removed branch July 26, 2017 12:00

dmanners mentioned this pull request Jul 26, 2017

Remove zend json from json controller #10342

Merged

4 tasks

korostii mentioned this pull request Jul 27, 2017

Admin Config Search #10335

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Zend1 Json from Magento Captcha module #8331

Remove Zend1 Json from Magento Captcha module #8331

dmanners commented Jan 30, 2017 •

edited by okorshenko

Loading

ihor-sviziev commented Jan 30, 2017 •

edited

Loading

dmanners commented Jan 30, 2017

dmanners commented Jan 30, 2017

buskamuza left a comment

dmanners commented Jan 31, 2017

dmanners commented Feb 2, 2017

ishakhsuvarov Feb 3, 2017

ishakhsuvarov Feb 3, 2017

ishakhsuvarov left a comment

dmanners commented Feb 3, 2017

ishakhsuvarov commented Feb 6, 2017

dmanners commented Feb 6, 2017

ishakhsuvarov commented Feb 7, 2017

ishakhsuvarov Feb 7, 2017

dmanners Feb 7, 2017

ishakhsuvarov Feb 7, 2017 •

edited

Loading

ishakhsuvarov Feb 7, 2017

ishakhsuvarov Feb 7, 2017

ishakhsuvarov Feb 7, 2017

okorshenko commented Feb 9, 2017

		$this->captchaHelper = $captchaHelper;
		$this->serializer = $serializer ?: \Magento\Framework\App\ObjectManager::getInstance()

Remove Zend1 Json from Magento Captcha module #8331

Remove Zend1 Json from Magento Captcha module #8331

Conversation

dmanners commented Jan 30, 2017 • edited by okorshenko Loading

ihor-sviziev commented Jan 30, 2017 • edited Loading

dmanners commented Jan 30, 2017

dmanners commented Jan 30, 2017

buskamuza left a comment

Choose a reason for hiding this comment

dmanners commented Jan 31, 2017

dmanners commented Feb 2, 2017

ishakhsuvarov Feb 3, 2017

Choose a reason for hiding this comment

ishakhsuvarov Feb 3, 2017

Choose a reason for hiding this comment

ishakhsuvarov left a comment

Choose a reason for hiding this comment

dmanners commented Feb 3, 2017

ishakhsuvarov commented Feb 6, 2017

dmanners commented Feb 6, 2017

ishakhsuvarov commented Feb 7, 2017

ishakhsuvarov Feb 7, 2017

Choose a reason for hiding this comment

dmanners Feb 7, 2017

Choose a reason for hiding this comment

ishakhsuvarov Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

ishakhsuvarov Feb 7, 2017

Choose a reason for hiding this comment

ishakhsuvarov Feb 7, 2017

Choose a reason for hiding this comment

ishakhsuvarov Feb 7, 2017

Choose a reason for hiding this comment

okorshenko commented Feb 9, 2017

dmanners commented Jan 30, 2017 •

edited by okorshenko

Loading

ihor-sviziev commented Jan 30, 2017 •

edited

Loading

ishakhsuvarov Feb 7, 2017 •

edited

Loading