Remove Zend1 captcha from Magento2 Captcha module #8356

dmanners · 2017-01-31T15:17:20Z

Since Zend1 has reached it's end of life we should migrate the Captcha module over to Zend2.

In this PR I have updated the class app/code/Magento/Captcha/Model/DefaultModel.php to extend \Zend\Captcha\Image rather than the Zend1 version.

I have updated the functions and variables that have changed between Zend1 and Zend2.

dmanners · 2017-01-31T15:30:54Z

Maybe outside of this request but I was also thinking about making the class DefaultModel as final. If someone wants to make a new Model then they can use the interface and build it from scratch, if they want to change the default then they can work with interceptors, I think that would work with a final class.

dmanners · 2017-01-31T18:24:45Z

The fails during the integration tests in php5.6 I think are down to configuration of Php5.6 gd on the travis envs. https://travis-ci.org/magento/magento2/jobs/196987191#L643

From the Zend point of view these exceptions have always been there and will cause an issue on the front end if the fonts are not there. The only change has been that the exceptions are now thrown in the constructor rather than a method that was never covered in the build process.

Unfortunately I would need some help debugging on the travis end but I can tell you on my PHP5.6 and PHP7 versions everything runs fine with linux 16 and not 14.

okorshenko · 2017-02-01T23:32:52Z

@dmanners Thank you for your contribution to Magento 2 project and updating outdated parts of our platform.

We contacted Travis Team support about this issue with the environment. Unfortunately, test image does not support freetypes (as a result imageftbbox is not available).
I confirm that we don't have this issue on our private testing environment. We will update you once Travis Support Team provides some recommendations.
Maybe outside of this request but I was also thinking about making the class DefaultModel as final

According to Magento 2 Backward compatibility policy, I wouldn't recommend make it final. In this case that will be Backward incompatible Change (BiC). All existing plugins for this class will be ignored, since it will be impossible to generate interceptor for this class.

dmanners · 2017-02-02T08:03:02Z

@okorshenko thanks for the feedback.

Easy reply first with regards to backwards compatibility I get it don't worry it was just food for thought. I nice read on the who "when to make classes final" is http://ocramius.github.io/blog/when-to-declare-classes-final/ but for Magento I get their point also.

With regards to the integration tests failing we could add in some setup checks during test setup for if the environment has the function or not. Then in this case the test would still run on PHP7 but not fail on PHP5.6 and then once the travis support has got back to you the test could automatically start running once the env has changed.

        if (!function_exists("imageftbbox")) {
            $this->markTestSkipped('This test is skipped as the enviournment does not allow for imageftbbox');
        }

Something like this could be added to either the individual test cases or the abstract test setup. Down side is that there would be a lot of tests that are skipped.

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

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

okorshenko

In general, I agree on solution

$this->markTestSkipped('This test is skipped as the enviournment does not allow for imageftbbox');

but we need to adjust this approach slightly.
Honestly, I don't think that we need app/code/Magento/Captcha/Model/DefaultModel.php
in every failed test. So we can try:
add new preference here https://github.com/magento/magento2/blob/develop/dev/tests/integration/etc/di/preferences/ce.php
Declare proxy for Magento/Captcha/Model/DefaultModel.php. In this way we will delay instantiation of this class in the integration tests. I hope that this will decrease the number of skipped tests.
Also, please, try to fix unit test

Thank you for your effort on this!
+100500 to your karma! :)

okorshenko · 2017-02-02T18:30:08Z

app/code/Magento/Captcha/Model/DefaultModel.php

@@ -517,7 +524,7 @@ protected function _randomSize()
     * Now deleting old captcha images make crontab script
     * @see \Magento\Captcha\Cron\DeleteExpiredImages::execute
     */
-    protected function _gc()
+    protected function gc()


Please, add SuppressWarnings here. Something like that:

Added SuppressWarnings since this method is declared in parent class and we can not use other method name. @SuppressWarnings(PHPMD.ShortMethodName)

dmanners · 2017-02-02T21:21:18Z

@okorshenko thanks for the feedback. I will have a look at this over the MageStackDay weekend. I just noticed there was more Zend1 that I probably cannot remove until the core DB lib has been changed :( but I can keep pushing some parts of the system!

dmanners · 2017-02-03T08:07:52Z

composer.json

@@ -31,6 +31,7 @@
        "zendframework/zend-serializer": "~2.4.6",
        "zendframework/zend-log": "~2.4.6",
        "zendframework/zend-http": "~2.4.6",
+        "zendframework/zend-captcha": "~2.4.6",
        "magento/zendframework1": "~1.12.16",


now my big question @okorshenko is who do we have to bribe/blackmail to get them to remove Captcha from this repo and create a new tag to be used in Mage2? Something like 2.0.0 would be nice here as this will fit well with http://semver.org/ as removing Captcha from zendframework1 would mean that this could not be used in Magento1.

okorshenko · 2017-02-07T23:01:40Z

@dmanners Just for your information
We got a response from Travis Support Team:

Hey Oleksii,
Thanks for reaching out and sorry for the troubles.
It's possible that our PHP 5.6 runtime for Trusty doesn't contain freetype. I will have to ask our Engineering Team. Sorry for the inconvenience.
In the meantime, can you try to run your builds on our Precise infrastructure to see if it helps? ?Here's what you need to add to your .travis.yml file to do so:

dist: precise

Unfortunately, their suggestion does not work for us since Precise infrastructure does not support mysql-server=5.6 (which is required by Magento 2). It supports 5.5 only.
I'm working with Travis Support Team to resolve this issue for you.

Thank you for your patience!

dmanners · 2017-02-09T07:32:04Z

Hi @okorshenko I have added the test version of the Default model that does not call the main constructor https://github.com/magento/magento2/pull/8356/files#diff-95faa1d786f0d7e3b6288e0d80f1583cR13 is there anything else that I can do with this PR as yet or is it sadly just a waiting game?

Thanks

…derscore at the start

…Captcha\Image value

…de of the parent method and as such we cannot control the name

…et around tests failing on travis as imageftbbox is not installed on the travis php5.6 image

…/TestFramework/Captcha/DefaultModel.php file

okorshenko · 2017-02-09T19:52:34Z

@dmanners thank you for the fix. I will do some adjustments for the code and merge this PR. Thank you!

okorshenko · 2017-02-12T04:18:39Z

@dmanners Zend1 captcha is removed from Magento2 Captcha by your contribution! Thank you!

shinesoftware · 2017-02-14T13:57:02Z

Hi guys,

I get this error in the last dev version:

PHP Fatal error:  Class 'Zend\Captcha\Image' not found in magento2.local/app/code/Magento/Captcha/Model/DefaultModel.php on line 13

How have I to solve it?

dmanners · 2017-02-14T13:58:34Z

hey @shinesoftware have you run a composer update since your last git pull?

shinesoftware · 2017-02-14T15:43:03Z

@dmanners solved thanks!

GraphQl. Reset mutable state after request

dmanners force-pushed the zend1-captcha-remove branch from f4229fb to c926ee4 Compare February 1, 2017 07:16

okorshenko self-requested a review February 1, 2017 15:06

maksek assigned okorshenko Feb 1, 2017

okorshenko added improvement Component: Captcha labels Feb 1, 2017

dmanners force-pushed the zend1-captcha-remove branch 2 times, most recently from beb391f to 3bfd544 Compare February 2, 2017 10:51

This was referenced Feb 2, 2017

Remove the usage of Zend_Db_Expr from the captcha module #8389

Closed

Remove Zend1 Json from Magento Captcha module #8331

Merged

okorshenko suggested changes Feb 2, 2017

View reviewed changes

dmanners commented Feb 3, 2017

View reviewed changes

dmanners force-pushed the zend1-captcha-remove branch from d717032 to 4c4b77a Compare February 3, 2017 08:41

dmanners added 8 commits February 9, 2017 07:44

Update Magento Captcha to use the Zend2 version of Captcha

1eb8d2b

Update protected method to private and update naming to remove the un…

1765350

…derscore at the start

Update the _gc method to be gc as this is now being used in generate

ffb0ae5

Fix type in note about fsize being overridden from the default \Zend\…

b547cdc

…Captcha\Image value

Add suppress warnings to captcha model's gc method. This is an overri…

5f9f2cd

…de of the parent method and as such we cannot control the name

Add a test framework version of Captcha/DefaultModel so that we can g…

7bf9f2e

…et around tests failing on travis as imageftbbox is not installed on the travis php5.6 image

Add newline to the end of the dev/tests/integration/framework/Magento…

fda4cfa

…/TestFramework/Captcha/DefaultModel.php file

Revert the file permissions on the composer.lock file

ee23cdd

dmanners force-pushed the zend1-captcha-remove branch from 902e741 to ee23cdd Compare February 9, 2017 07:46

okorshenko approved these changes Feb 9, 2017

View reviewed changes

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

okorshenko added the Progress: accept label Feb 11, 2017

mmansoor-magento merged commit ee23cdd into magento:develop Feb 11, 2017

dmanners mentioned this pull request Feb 13, 2017

Change in the Magento Zend1 Framework #8527

Closed

vrann added the Award: advanced label Mar 11, 2017

dmanners deleted the zend1-captcha-remove branch July 26, 2017 12:00

dmanners mentioned this pull request Jan 26, 2018

Cleanup and PHP 7.2 Support magento-engcom/zf1-php-7.2-support#1

Closed

magento-devops-reposync-svc pushed a commit that referenced this pull request Sep 8, 2023

Merge pull request #8356 from magento-performance/application-server

05fa3de

GraphQl. Reset mutable state after request

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Zend1 captcha from Magento2 Captcha module #8356

Remove Zend1 captcha from Magento2 Captcha module #8356

dmanners commented Jan 31, 2017 •

edited by okorshenko

Loading

dmanners commented Jan 31, 2017

dmanners commented Jan 31, 2017

okorshenko commented Feb 1, 2017 •

edited

Loading

dmanners commented Feb 2, 2017 •

edited

Loading

dmanners commented Feb 2, 2017

okorshenko left a comment •

edited

Loading

okorshenko Feb 2, 2017

dmanners commented Feb 2, 2017

dmanners Feb 3, 2017

okorshenko commented Feb 7, 2017

dmanners commented Feb 9, 2017

okorshenko commented Feb 9, 2017

okorshenko commented Feb 12, 2017

shinesoftware commented Feb 14, 2017

dmanners commented Feb 14, 2017

shinesoftware commented Feb 14, 2017

Remove Zend1 captcha from Magento2 Captcha module #8356

Remove Zend1 captcha from Magento2 Captcha module #8356

Conversation

dmanners commented Jan 31, 2017 • edited by okorshenko Loading

dmanners commented Jan 31, 2017

dmanners commented Jan 31, 2017

okorshenko commented Feb 1, 2017 • edited Loading

dmanners commented Feb 2, 2017 • edited Loading

dmanners commented Feb 2, 2017

okorshenko left a comment • edited Loading

Choose a reason for hiding this comment

okorshenko Feb 2, 2017

Choose a reason for hiding this comment

dmanners commented Feb 2, 2017

dmanners Feb 3, 2017

Choose a reason for hiding this comment

okorshenko commented Feb 7, 2017

dmanners commented Feb 9, 2017

okorshenko commented Feb 9, 2017

okorshenko commented Feb 12, 2017

shinesoftware commented Feb 14, 2017

dmanners commented Feb 14, 2017

shinesoftware commented Feb 14, 2017

dmanners commented Jan 31, 2017 •

edited by okorshenko

Loading

okorshenko commented Feb 1, 2017 •

edited

Loading

dmanners commented Feb 2, 2017 •

edited

Loading

okorshenko left a comment •

edited

Loading