Make email validation only validate @ sign presence, fixes #4547 #9087

sambolek · 2017-04-01T14:32:26Z

Description

New TLDs aren't valid because of Zend Framework Email validator, which is too strict. As some pointed out, this might not be necessary. See discussion: https://github.com/magento/magento2/pull/8420/files#r99476657

Fixed Issues (if relevant)

Support for new .tech TLD and others required #4547: Support for new .tech TLD and others required

Manual testing scenarios

During customer registration, try to register with email test@test.swiss
Observe an error being thrown http://prntscr.com/er5qdb
"Email" is not a valid hostname. 'test.swiss' looks like a DNS hostname but we cannot match the TLD against known list. 'test.swiss' looks like a local network name, which is not an acceptable format.

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)

sambolek · 2017-04-01T16:09:55Z

Will fix build errors in case this pull request is considered a good way forward. Awaiting comments.

orlangur · 2017-04-01T20:28:58Z

Cool! 👍 Especially that you didn't forget about a JavaScript part.

I'm adding some comments for current changeset and let's proceed after we have an official confirmation such approach is generally acceptable.

orlangur · 2017-04-01T20:51:09Z

app/code/Magento/Customer/Model/Metadata/Form/AbstractData.php

@@ -336,42 +336,42 @@ protected function _validateInputRule($value)
                    __("'%value%' appears to be a DNS hostname but cannot extract TLD part")
                    __("'%value%' appears to be a DNS hostname but cannot match TLD against known list")
                    */
-                    $validator = new \Zend_Validate_EmailAddress();
+                    $validator = new \Magento\Framework\Validator\EmailAddress();


We won't have anything besides "%1" is not a valid email address. validator error anymore thus all irrelevant cases shall be removed.

orlangur · 2017-04-01T20:51:55Z

app/code/Magento/Eav/Model/Attribute/Data/AbstractData.php

@@ -364,42 +364,42 @@ protected function _validateInputRule($value)
                    __("'%value%' appears to be a DNS hostname but cannot extract TLD part")
                    __("'%value%' appears to be a DNS hostname but cannot match TLD against known list")
                    */
-                    $validator = new \Zend_Validate_EmailAddress();
+                    $validator = new \Magento\Framework\Validator\EmailAddress();


We won't have anything besides "%1" is not a valid email address. validator error anymore thus all irrelevant cases shall be removed.

orlangur · 2017-04-01T20:54:54Z

app/code/Magento/Ui/view/base/web/js/lib/validation/rules.js

            },
            $.mage.__('Please enter a valid email address (Ex: johndoe@domain.com).')
        ],
        'validate-emailSender': [
            function (value) {
-                return utils.isEmptyNoTrim(value) || /^[\S ]+$/.test(value);
+                return utils.isEmptyNoTrim(value) || value.indexOf('@') !== -1;


After such change there is no difference between validate-email and validate-emailSender.

I believe it is wrongly copy-pasted from the bottom file:

['validate-emailSender', 'Please use only visible characters and spaces.', function (v) { return Validation.get('IsEmpty').test(v) || /^[\S ]+$/.test(v) return Validation.get('IsEmpty').test(v) || /^[\S ]+$/.test(v)

So, please just fix message and do not change validation function here.

Please fix misprint with validate-emailSender if you don't mind (or tell me and I'll report it separately if you believe it is out of scope).

orlangur · 2017-04-01T20:58:13Z

lib/internal/Magento/Framework/Validator/EmailAddress.php

@@ -9,4 +9,13 @@

 class EmailAddress extends \Zend_Validate_EmailAddress implements \Magento\Framework\Validator\ValidatorInterface


There is no need to keep inheritance from Zend Framework 1 class. Please mark this class as @deprecated without changes and create a new one with proper logic.

As we don't have EmailAnything except EmailAddress, I believe simply Email would be a good name for such class.

orlangur · 2017-04-01T21:05:10Z

lib/internal/Magento/Framework/Validator/EmailAddress.php

@@ -9,4 +9,13 @@

 class EmailAddress extends \Zend_Validate_EmailAddress implements \Magento\Framework\Validator\ValidatorInterface
 {
+    public function isValid($value)
+    {
+        if (false === \strpos($value, '@')) {


Please avoid Yoda conditions - JavaScript logic you wrote is much more readable ;) What would be really useful here is unit test

Remove leading backslash for function, there is no static test which forbids or enforces this but there is no other occurrence of \strpos currently. Let's be consistent.

strrpos will work faster than strpos at average as domain is usually shorter than account name ;D

orlangur · 2017-04-01T21:06:26Z

lib/web/mage/validation.js

            },
            $.mage.__('Please enter a valid email address (Ex: johndoe@domain.com).')
        ],
        'validate-emailSender': [
            function (v) {
-                return $.mage.isEmptyNoTrim(v) || /^[\S ]+$/.test(v);
+                return $.mage.isEmptyNoTrim(v) || v.indexOf('@') !== -1;


Same as https://github.com/magento/magento2/pull/9087/files#r109294015

…r to other Magento validators, fix code errors

sambolek · 2017-04-02T19:27:13Z

Hi @orlangur
created a new validator for Emails:
https://github.com/sambolek/magento2/blob/dc8867d40ca763a0a11012b1700659251b5ed378/lib/internal/Magento/Framework/Validator/Email.php

that follows Currency validator practice:
https://github.com/sambolek/magento2/blob/dc8867d40ca763a0a11012b1700659251b5ed378/lib/internal/Magento/Framework/Validator/Currency.php

Is this okay, or should I look into how AllowedProcotols validator is implemented, and have Email validator extend from AbstractValidator and implement ValidatorInteface as well, like here: https://github.com/sambolek/magento2/blob/dc8867d40ca763a0a11012b1700659251b5ed378/lib/internal/Magento/Framework/Validator/AllowedProtocols.php ?

…nteface

sambolek · 2017-04-02T20:04:32Z

Attempting to implement this validator by extending AbstractValidator and implementing ValidatorInteface.

As AbstractValidator is missing some key components to compete with Zend_Validate_Abstract, I have added some methods to this Email validator, so we can add messages by key, like we can for all Zend validators.

If we're looking to implement our own validators for some parts of the system, moving forward, we should have an implementation for setMessage, _error, and _createMessage in AbstractValidator. That way, adding new validators will be easier.

…n email address

orlangur · 2017-04-03T07:57:54Z

@sambolek thanks, I'll look deeper into implementation today and share my thoughts.

Anticipatorily I believe we should not have anything ZF-specific like setMessage outside the Magento Framework thus such email-related code in core modules shall be adopted to our own narrowed validator interface. But I can change my mind after looking into existing implementation.

Also, we need to think about ZF1 validators replacement. I'll check how ZF2 validators differ from ZF1, for instance.

sambolek · 2017-04-05T22:09:18Z

@orlangur any news on this one?

orlangur · 2017-04-06T06:13:02Z

@sambolek sorry, forgot to review that evening and then had no time. Will review asap today and this time will not forget.

orlangur · 2017-04-10T10:02:01Z

Hi @sambolek! I left some comments in review and let me clarify approach I find the best one here.

that follows Currency validator practice:
https://github.com/sambolek/magento2/blob/dc8867d40ca763a0a11012b1700659251b5ed378/lib/internal/Magento/Framework/Validator/Currency.php

This one is a bad example I believe, it does not implement any interface in fact :D

I believe when the \Magento\Framework\Validator\ValidatorInterface was introduced additional methods like setMessage or error were intentionally omitted. For this simplified to teh max validator we don't need such infrastructure and it seems a good idea to me to encapsulate the ultimate possible validation error message within validator.

The closest good example I see is \Magento\Framework\Validator\DataObject - you need just to implement isValid and getMessages methods. Maybe we need to extend \Magento\Framework\Validator\AbstractValidator instead of just implementing \Zend_Validate_Interface as DataObject does, but as I didn't spot any consistent pattern in class/interface hierarchy of validators I leave such decision up to you.

orlangur · 2017-04-10T09:45:52Z

app/code/Magento/Customer/Model/Metadata/Form/AbstractData.php

-                            "'%value%' appears to be a DNS hostname, but the given punycode notation cannot be decoded."
-                        ),
-                        \Zend_Validate_Hostname::CANNOT_DECODE_PUNYCODE
+                        \Magento\Framework\Validator\Email::INVALID


Please encapsulate it in validator.

orlangur · 2017-04-10T09:47:22Z

app/code/Magento/Ui/view/base/web/js/lib/validation/rules.js

            },
            $.mage.__('Please enter a valid email address (Ex: johndoe@domain.com).')
        ],
        'validate-emailSender': [
            function (value) {
-                return utils.isEmptyNoTrim(value) || /^[\S ]+$/.test(value);
+                return utils.isEmptyNoTrim(value) || value.indexOf('@') !== -1;


Please fix misprint with validate-emailSender if you don't mind (or tell me and I'll report it separately if you believe it is out of scope).

orlangur · 2017-04-10T09:49:07Z

lib/internal/Magento/Framework/Validator/Email.php

+ */
+namespace Magento\Framework\Validator;
+
+class Email extends AbstractValidator implements \Magento\Framework\Validator\ValidatorInterface


I believe the plan was to avoid inheritance from AbstractValidator whenever possible, by just implementing a couple of \Zend_Validate_Interface methods.

schmengler · 2017-04-22T21:36:54Z

Thank you for your contribution! Can you adjust the failing tests to the new implementation? And please revert the changes in jQuery, modifications of third party libraries should not be allowed (see also: #7593 (comment))

So we either have to live with that regex validation or replace its usages.

orlangur · 2017-04-22T22:32:35Z

modifications of third party libraries should not be allowed

Is it officially prohibited somewhere? As to me changing third-party libraries behavior is perfectly fine until we do not break upgradeability.

Probably, we can move this change to the bottom of file so that it is consistent with other basic functionality extensions there. But moving such change to a separate file is not a good idea as third-party developers can accidentally use original jQuery version instead of patched one.

chreds · 2017-06-28T22:03:47Z

Any movement on this? Lots of new TLD's that aren't supported in Magento2 right now.

orlangur · 2017-06-30T11:25:20Z

@chreds yes. As there are two possible implementation approaches an approval from Magento Product Team was requested to make an informed decision. After that we will be able to proceed with this PR acceptance.

okorshenko · 2017-07-21T15:59:49Z

Hi @orlangur and @sambolek Thank you for working on this issue. ZF2 resolves the problem described in #4547 (see hostname validator and support of IANA)

We would like to update validator library from ZF1 to ZF2 in the entire application and eliminate direct knowledge about ZF1 validator from the code base. We should introduce Magento adapters and interfaces (where needed) and use only Magento code though the code base.

Update of ZF1 validator to ZF2 is a big task and we would like to have it done for sure.
Please, let me know if you would like to contribute to Magento upgrade of this component and we will start working with you on this upgrade. We already have several similar upgrade tasks that were implemented by Magento Community. See the project Up For Grabs

Closing this PR. We would like to avoid the validation only by @ sign.
Thanks everyone for collaboration!

orlangur · 2017-07-21T17:31:00Z

@okorshenko that's sad to hear as validating only @ is the best approach as to me and will never break down again :) Thanks for the reply though!

Don't forget to report https://github.com/magento/magento2/pull/8420/files#r99476657 as technical debt then ;)

korostii · 2017-07-24T10:10:06Z

We would like to avoid the validation only by @ sign.

Could you please share the motivation for this decision?

IMHO it is much more acceptable from UX perspective to have some invalid emails pass through rather than deny a customer from using a perfectly valid address. And thus having a less restrictive validator is much better than having a broken one.
Please also see the article referenced by @schmengler: https://hackernoon.com/the-100-correct-way-to-validate-email-addresses-7c4818f24643

It puzzles me: why not use this one at least temporarily, until a Magento-style (c), adapter-wrapped, ZF2 validator is actually implemented (which might take months or years for all we know)

[Support Tier-4 aplapana] 07-16-2024 Regular delivery of bugfixes and improvements

Make email validation only validate @ sign presence, fixes #4547

c736224

sambolek closed this Apr 1, 2017

sambolek reopened this Apr 1, 2017

orlangur requested changes Apr 1, 2017

View reviewed changes

sambolek added 3 commits April 2, 2017 20:37

Merge remote-tracking branch 'upstream/develop' into issue-4547

6ba09d6

Remove dependency on Zend validator, make validator logic more simila…

dc8867d

…r to other Magento validators, fix code errors

Add obsolete EmailAddress validator class

4a3612f

Attempt to create Email validator by implementing AbstractValidator i…

196994d

…nteface

sambolek added 7 commits April 2, 2017 22:10

Fix exception, use Phrase instead of plain string

c6fbf5c

Fix logical error

5d7d971

Fix logical error

2775007

Unify messages

a476b05

Fix integration test failure, we no longer check for valid hostname i…

c8239e0

…n email address

Fix failing static tests

9263b0e

Fix phpcs & phpmd warnings

9d9be7d

orlangur requested changes Apr 10, 2017

View reviewed changes

sambolek added 2 commits April 14, 2017 13:44

Merge remote-tracking branch 'upstream/develop' into issue-4547

2ef2dfd

Revert emailSender js validator change

764f757

korostii mentioned this pull request Apr 25, 2017

Unable to save shipping information. Please check input data. #8544

Closed

orlangur self-assigned this Jun 8, 2017

orlangur added this to the June 2017 milestone Jun 8, 2017

orlangur added the Progress: needs update label Jun 8, 2017

sambolek added 2 commits June 16, 2017 23:16

Merge remote-tracking branch 'upstream/develop' into issue-4547

6e3f8e7

Implement EmailMinimal validator, replace usages of old Email validator

0095a1b

okorshenko modified the milestones: June 2017, July 2017 Jul 2, 2017

okorshenko self-assigned this Jul 21, 2017

okorshenko closed this Jul 21, 2017

magento-devops-reposync-svc pushed a commit that referenced this pull request Jul 25, 2024

Merge pull request #9087 from adobe-commerce-tier-4/PR-2024-07-11

b21e5d9

[Support Tier-4 aplapana] 07-16-2024 Regular delivery of bugfixes and improvements

		@@ -9,4 +9,13 @@

		class EmailAddress extends \Zend_Validate_EmailAddress implements \Magento\Framework\Validator\ValidatorInterface

Make email validation only validate @ sign presence, fixes #4547 #9087

Make email validation only validate @ sign presence, fixes #4547 #9087

Uh oh!

Conversation

sambolek commented Apr 1, 2017

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

sambolek commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sambolek commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sambolek commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Apr 3, 2017

Uh oh!

sambolek commented Apr 5, 2017

Uh oh!

orlangur commented Apr 6, 2017

Uh oh!

orlangur commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmengler commented Apr 22, 2017

Uh oh!

orlangur commented Apr 22, 2017

Uh oh!

chreds commented Jun 28, 2017

Uh oh!

orlangur commented Jun 30, 2017

Uh oh!

okorshenko commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Jul 21, 2017

Uh oh!

korostii commented Jul 24, 2017

Uh oh!

Uh oh!

sambolek commented Apr 1, 2017 •

edited

Loading

sambolek commented Apr 2, 2017 •

edited

Loading

sambolek commented Apr 2, 2017 •

edited

Loading

okorshenko commented Jul 21, 2017 •

edited

Loading