[Validator] #18156 In "strict" mode, email validator inaccurately cla…

[natechicago]

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

Email validation can now occur in one of four different modes/profiles:

• Basic regex
• HTML5 regex
• RFC-compliant (non-strict; informational warnings raised during validation are ignored)
• RFC-compliant (strict; warnings will cause an otherwise-valid email to be considered invalid)

The legacy "strict" mode has been marked as deprecated, but for now is still fully supported.

[backbone87]

$profile should default to null, use $strict only if $profile is null. so strict can be ignored, if profile is given.

public function __construct($strict = false, $profile = null)
{
    $this->isStrict = $strict;
    $this->defaultProfile = $profile === null
        ? $strict
            ? Email::PROFILE_RFC_DISALLOW_WARNINGS
            : Email::PROFILE_BASIC_REGX
        : $profile;
}

[natechicago]

This is much better, as my original implementation would have misbehaved in the edge case situation with both config options set (strict_email = TRUE, and email_profile='html5'). Thanks for catching that!

[natechicago]

I'm not quite sure why the new tests are failing under Travis CI's PHP 7. They don't contain any controversial code, and they pass without any problems in my local PHP 7 environment.

[xabbuh]

@natechicago I guess the failure is related to the minimal allowed version of the validation library (which will be used in the PHP 7 build) and the fact that you use addresses with parentheses/brackets (there have been some recent changes in the validator library fixing issues with them). Would it be possible to choose other example data for the tests that will pass with older versions of the library?

[natechicago]

@xabbuh thanks for pointing me in the right direction! The tests are now compliant with the minimal allowed version of the egulias/email-validation library.

[xabbuh]

should be on one line

[backbone87]

the failing test is unrelated to this ticket, can someone review?

[xabbuh]

The removal of the strict option must be documented in the UPGRADE_4.0.md file.

[xabbuh]

And we will need to trigger deprecations.

[ro0NL]

What about custom profiles? Perhaps implement protected function validateProfile($profile)?

[ro0NL]

What about a EmailProfileInterface..?

[backbone87]

@ro0NL i think thats too much overengineering

[ro0NL]

@backbone87 agree 👍 custom validation can be done with an additional constraint anyways.

Still.. the EmailValidator class isn't really extendable right now to allow easy built-in customization, i.e. the addition of protected function validateProfile($profile) (or to be consistent; checkProfile) would still be useful imho. Same goes for changing the visibility ofcheckMX and checkHost from private to protected, that would improve extending significantly. What do you think?

[javiereguiluz]

I'm -1 to this proposal. My reasons:

1. Validation is a binary thing: either the email address is valid or it's not.
   But this PR proposes four different behaviors: maybe it's valid (basic), probably it's valid (html5), almost certainly it's valid but I'll ignore errors (rfc non-strict) and almost certainly it's valid and I'll trigger errors (rfc strict).

2. The decision to add all kinds of validation variations smells like a "decision by committee", which is one of the worst things you can do when developing a feature.

3. Some developers refuse to accept that there are problems which cannot be solved with code. But that's a fact, and validating email addresses is one of the best examples.

4. Even with the four proposed validations, millions of valid email addresses will be considered invalid. They are the infamous Japanese emails which are "technically wrong" but "fully working" in the real world (see this and this for details).

---

In #18177 I proposed an alternative solution:

• Simplify everything to provide just a regexp-based validator which validates
  emails as "probably valid".
• If you want 100% certainty, send a confirmation email message.

[ro0NL]

I upvoted #18177 :-) Do you agree on profiles as a nice to have or should symfony force you with simple regex validation only, and "hey.. build something to send a confirmation email" or use a custom callback/regexp/whatever constraint additionally.

I personally favor a simple regex by default, i.e. the basic profile (/^.+\@\S+\.\S+$/) and a confirmation email at application-level. But still like to see these additional profiles being available. Perhaps more explicit in separate classes, but that would be overengineering things.

Edit: even the basic profile is to restricted on the dot part imho... =/

Edit 2: my point is.. even if we use the HTML5 regex as you proposed in #18177 than the EmailValidator could still produce false negatives right? (e.g. the weird japanese emails you mentioned), i.e. whats the goal for the EmailValidator? To be really generic so we avoid any false negatives/positives whatsoever, being technically correct, or being both?

[egulias]

Hello again.
Just to let you know there's a roadmap for v2 of the library (https://github.com/egulias/EmailValidator/milestones/v2.0.0) where you will be able to use any validation "strategy" (egulias/EmailValidator#62) you like. RFC strictness will become one of such strategies (like DNS check , HTML5 regexp or even filter_var) to allow for extension and customization.
I'll be happy to also do the PR to Symfony to add the new version and the configuration need, something like
validator: email_validator:[HTML5Validation, DNSValidation]
This way knowing, customizing and using the email validation would be easier.
Also take into account using any form of RegExp:

• will not allow UTF8 emails (not just Japanese)
• will not work with Swiftmailer <=5 (uses a quite complex RegExp) and 6 uses EmailValidator same validator.

@ro0NL the initial idea was to be technically correct , with v2 I'm aiming to be both.
@javiereguiluz issue swiftmailer/swiftmailer#71 from swiftmailer was closed with the use, in v6, of the EmailValidator lib we are here trying to find out if it is useful or not.

[egulias]

FYI
API proposal for isValid

[fabpot]

I'm also 👎 on adding "email validation strategies". We decided to rely on an external library for email validation and we should stick with whatever they provide. In any case, someone can create their own email validator if that makes sense in their use case, no need to change Symfony's internal one.