[Validator] Add common double dot email typo #24429

PurpleBooth · Oct 4, 2017

The most common email typo that we see in our logs is the "double dot".
This is where a user types "username@example..com", rather than
"username@example.com". This particular common user typo isn't a
possible email address and, wasn't previously covered by the validator.

Why is this added here and not strict? Strict allows some addresses like
"username@example", which, while technically valid (you could use this
case to address a machine with the hostname "example") are undesirable
for nearly all applications.

Note: This specific case is already covered under strict validations.

Q	A
Branch?	3.4 or master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets
License	MIT
Doc PR

PurpleBooth · Oct 4, 2017

I'm unsure why the the Appveyor tests are failing. It doesn't seem to be related to the code changes.

PurpleBooth · Oct 5, 2017

I've just changed this to support the cases.

example@example..com
example@example.co..uk
example@example..co.uk
example@example..co..uk

ro0NL · Oct 5, 2017

src/Symfony/Component/Validator/Constraints/EmailValidator.php

@@ -78,7 +78,7 @@ public function validate($value, Constraint $constraint)

                return;
            }
-        } elseif (!preg_match('/^.+\@\S+\.\S+$/', $value)) {
+        } elseif (!preg_match('/^.+\@[^\s.]+(\.[^\s.]+)+$/', $value)) {


(?:...) (non-capturing)

Ooh! Good catch. I've added that in

linaori · Oct 5, 2017

I've pulled up a list of email addresses that my company has added to a custom email checker, which are not all caught by the non-strict Symfony checker. Not saying they must be added, but it would be nice to have even more checks, making the checker more complete.

array('a@a'),
array('a@a.'),
array('a@.nl'),
array('@a.nl'),
array('.email@with.a.domain'),
array('email.@with.a.domain'),
array('.email.@with.a.domain'),
array('em..ail@with.a.domain'),
array('a@a.nl;a@a.nl'),
array('a@'),
array('a@.'),
array(' a@a.nl'),
array('a@ '),
array(' a@a.nl '),
array(' a @a .nl '),
array('a@-invalid.hostname'),
array('email@with.averylongextensionbehindthedomain'),
array('email-with-dashes@a.a'),

nicolas-grekas · Oct 5, 2017

I don't know if we can agree on adding this rule as bug fix: on one side, I really think rejecting such emails should have been the case. On the other side, this is a BC break that may make apps display new errors to end users, or break some cron job, isn't?

I would strongly suggest to move to HTML5 validation by default in Symfony 4.0:

/^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/

(we should just change the last * by a + to enforce a least one dot in the domain part.)

We need to find a way to make this move and get rid of the current very loose validation in 4.0.
What about throwing, in 3.4, a deprecation notice when an email doesn't pass the HTML5 validation?
That HTML5 regexp wouldn't be enforced and we would still use the current one, but then, in 4.0, we could enforce it and devs would have a way to get some notices about the issue.
Yes that wouldn't fix broken emails, but they are broken anyway, so I think we can do it in a new major version.

WDYT @symfony/deciders?

@PurpleBooth would you be up to work on this in the next days/weeks?

linaori · Oct 5, 2017

Sounds like a good idea, I like it!

PurpleBooth · Oct 5, 2017

Sure, I'm totally open to doing that.

Just to be clear:

v4 - Use HTML5 Email Regex
v3.4 - Raise depreciation notice when HTML5 Pattern isn't met?

nicolas-grekas · Oct 5, 2017

@PurpleBooth correct

xabbuh · Oct 5, 2017

Triggering a deprecation based on the input does not sound like a good idea to me. That's too fragile and likely to not be detected during development. What about instead introducing a mode option that could be something like html5 or rfc(in fact, we could allow several RFC types to support the different validation schemes supported by EmailValidator 2)? This option could default to a special loose value in 3.4 to use the old simple regex.

nicolas-grekas · Oct 5, 2017

@xabbuh that's where we stopped at in previous discussions, by consideration for broken emails. That's why I'm proposing this simple plan. Yes, it's a pushy change. But I think it will just work and we could move forward faster and make apps better by default, instead of having to play with the deprecation policy to 1. add an option, 2. for setting it, 3. deprecate it, 4. remove it, etc.
Do you really think we should follow this path? I'm not sure. But if we want, we need to actually do it this time.
I still prefer the plan I proposed :)

xabbuh · Oct 5, 2017

@nicolas-grekas Well, deprecation based on user input is what looks flaky to me. I am afraid that this calls for bug reports in the future. And IIRC we already had some feature requests in the past to support more validation rules besides NoRFCWarningsValidation. So that would be the opportunity to make that happen as well.

nicolas-grekas · Oct 5, 2017

@xabbuh would you be up to make that happen very soon (because feat freeze?)

nicolas-grekas · Oct 5, 2017

Or @PurpleBooth of course?

xabbuh · Oct 5, 2017

I can try to come up with something tonight. But I can't promise anything. So if someone can create a PR before, that would be nice.

PurpleBooth · Oct 5, 2017

Would the mode option imply the need for EmailValidator, or would that always be required now?

PurpleBooth · Oct 5, 2017

I've put together what I understand as the "mode" option. It needs work, but for discussion purposes #24442

PurpleBooth · Oct 6, 2017

I've changed this to deprecate the loose pattern and raise a E_DEPRECATED as discussed. I don't really think this is a good solution as it doesn't offer people a intermediary fix path as has already been mentioned.

I much prefer the mode solution in #24442

The most common email typo that we see in our logs is the "double dot". This is where a user types "username@example..com", rather than "username@example.com". This particular common user typo isn't a possible email address and, wasn't previously covered by the validator. Why is this added here and not strict? Strict allows some addresses like "username@example", which, while technically valid (you could use this case to address a machine with the hostname "example") are undesirable for nearly all applications. Note: This specific case is already covered under strict validations.

nicolas-grekas · Oct 8, 2017

Closing in favor of #24442 then. Thanks for raising the point.

carsonbot added Status: Needs Review Feature labels Oct 4, 2017

PurpleBooth force-pushed the prevent-double-dot branch 2 times, most recently from bff80cd to 65fe4bd Compare October 4, 2017 22:04

PurpleBooth changed the title ~~Validators: Add common email typo~~ [Validators] Add common double dot email typo Oct 4, 2017

PurpleBooth changed the title ~~[Validators] Add common double dot email typo~~ [Validator] Add common double dot email typo Oct 4, 2017

PurpleBooth force-pushed the prevent-double-dot branch from 65fe4bd to ab63bb0 Compare October 4, 2017 22:08

PurpleBooth force-pushed the prevent-double-dot branch from ab63bb0 to ca273b3 Compare October 4, 2017 22:39

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 5, 2017

javiereguiluz approved these changes Oct 5, 2017

View reviewed changes

PurpleBooth force-pushed the prevent-double-dot branch from ca273b3 to 5a52e81 Compare October 5, 2017 07:05

ro0NL reviewed Oct 5, 2017

View reviewed changes

PurpleBooth force-pushed the prevent-double-dot branch from 5a52e81 to 5c88073 Compare October 5, 2017 07:34

ro0NL approved these changes Oct 5, 2017

View reviewed changes

xabbuh added the Validator label Oct 5, 2017

PurpleBooth changed the base branch from master to 3.4 October 6, 2017 06:56

PurpleBooth force-pushed the prevent-double-dot branch 2 times, most recently from e9adf60 to 55da378 Compare October 6, 2017 07:09

PurpleBooth force-pushed the prevent-double-dot branch from 55da378 to 7daaa69 Compare October 6, 2017 09:54

nicolas-grekas closed this Oct 8, 2017

nicolas-grekas added this to the 3.4 milestone Oct 8, 2017

PurpleBooth deleted the prevent-double-dot branch October 8, 2017 20:17

Search code, repositories, users, issues, pull requests...

Uh oh!

[Validator] Add common double dot email typo #24429

[Validator] Add common double dot email typo #24429

Uh oh!

Conversation

PurpleBooth commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PurpleBooth commented Oct 4, 2017

Uh oh!

PurpleBooth commented Oct 5, 2017

Uh oh!

ro0NL Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

PurpleBooth Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

linaori commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linaori commented Oct 5, 2017

Uh oh!

PurpleBooth commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

xabbuh commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

xabbuh commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

xabbuh commented Oct 5, 2017

Uh oh!

PurpleBooth commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PurpleBooth commented Oct 5, 2017

Uh oh!

PurpleBooth commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Oct 8, 2017

Uh oh!

Uh oh!

PurpleBooth commented Oct 4, 2017 •

edited

Loading

nicolas-grekas commented Oct 5, 2017 •

edited

Loading

PurpleBooth commented Oct 5, 2017 •

edited

Loading

PurpleBooth commented Oct 5, 2017 •

edited

Loading

PurpleBooth commented Oct 6, 2017 •

edited

Loading