[Validator] Html5 Email Validation #24442

PurpleBooth · 2017-10-05T15:57:03Z

Currently we only support a very loose validation. There is now a
standard HTML5 element with matching regex. This will add the ability
to set a mode on the email validator. The mode will change the
validation that is applied to the field as a whole.

These modes are:

loose: The pattern from previous Symfony versions (default)
strict: Strictly matching the RFC
html5: The regex used for the HTML5 Element

Deprecates the strict=true parameter in favour of mode='strict'

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#21531
License	MIT
Doc PR	symfony/symfony-docs#8487

nicolas-grekas

thanks for the PR!
some pure style comments for now to help next reviews
this PR should target 3.4 (rebase+github-retarget needed)

nicolas-grekas · 2017-10-05T16:05:08Z

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


-    public function __construct($strict = false)
+    public function __construct($mode = 'html5')


we need to make the change in a BC friendly way, so I think we cannot change the default behavior
true/false should still be handled as before as much as this makes sense

Something along these lines?

/** * @var string */ private $mode; public function __construct($strict = false, $mode = 'html5') { $this->mode = $mode; if($strict === true) { $this->mode = Email::VALIDATION_MODE_STRICT; } }

No, we should have only 1 argument (as we don't want to keep a first unused argument in the future API)

nicolas-grekas · 2017-10-05T16:06:16Z

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

-                    ->setParameter('{{ value }}', $this->formatValue($value))
-                    ->setCode(Email::INVALID_FORMAT_ERROR)
-                    ->addViolation();
+                              ->setParameter('{{ value }}', $this->formatValue($value))


please use single indentation level as before, we don't align ->

nicolas-grekas · 2017-10-05T16:06:54Z

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


                return;
-            } elseif (!interface_exists(EmailValidation::class) && !$strictValidator->isValid($value, false, true)) {


I would keep "strict" instead of "rfc" for maintenance purpose (will help when merging branches)

nicolas-grekas · 2017-10-05T16:07:15Z

src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php

@@ -54,7 +54,6 @@ public function testExpectsStringCompatibleType()
    public function testValidEmails($email)
    {
        $this->validator->validate($email, new Email());
-


should be revert, to ease reviewing the patch

nicolas-grekas · 2017-10-05T16:07:40Z

src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php

@@ -94,23 +93,23 @@ public function getInvalidEmails()
        );
    }

-    public function testStrict()
+    public function testRfc()


same: let's keep strict everywhere to reduce the diff, thus help the review+future merges

stof

Note having any test for the new mode is wrong.

stof · 2017-10-05T16:19:24Z

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

        }

-        if ($constraint->strict) {
+        var_dump($constraint->mode);


should be removed.

stof · 2017-10-05T16:20:42Z

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


                return;
            }
-        } elseif (!preg_match('/^.+\@\S+\.\S+$/', $value)) {
+        } elseif (Email::VALIDATION_MODE_LOOSE === $constraint->mode && preg_match('/^.+\@\S+\.\S+$/', $value)) {


this is broken. You removed the !, so considering any email with a @ in it as invalid.

stof · 2017-10-05T16:20:57Z

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

+                          ->addViolation();
+
+            return;
+        } elseif (Email::VALIDATION_MODE_HTML5 === $constraint->mode && preg_match(


missing negation too

stof · 2017-10-05T16:27:43Z

src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php

    {
-        $constraint = new Email(array('strict' => true));


Some tests must be kept with the existing name, to cover backward compatibility (if this is deprecated, it should be come a legacy test, but it should not disappear)

stof · 2017-10-05T16:29:18Z

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

    public $message = 'This value is not a valid email address.';
    public $checkMX = false;
    public $checkHost = false;
-    public $strict;
+    public $mode = 'html5';


This is a BC break for 3 reasons:

the strict property does not work anymore

the default behavior has changed)

the constraint now has a default mode, meaning it is not possible to configure a global default anymore at the config level (as the validator uses it only when the constraint does not have a config)

PurpleBooth · 2017-10-05T16:51:17Z

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

+        $this->defaultMode = $defaultMode;
+
+        if (true === $strict) {
+            $this->defaultMode = Email::VALIDATION_MODE_STRICT;


Not terribly happy with this. Suggestions?

something like that yes (sorry for the CS)

public function __construct($defaultMode = Email::VALIDATION_MODE_STRICT) { switch ($defaultMode) { case VALIDATION_MODE_HTML5: case VALIDATION_MODE_STRICT: case VALIDATION_MODE_LOOSE: break; default: $defaultMode = $defaultMode ? VALIDATION_MODE_STRICT : VALIDATION_MODE_LOOSE; } $this->defaultMode = $defaultMode; }

PurpleBooth · 2017-10-29T11:15:11Z

Failing tests come from current 3.4 branch (as of 3.4 being at 98dae3e).

nicolas-grekas · 2017-10-30T14:05:57Z

We talked about including this on 3.4 and decided it was too late. As such, moving to 4.1.

PurpleBooth · 2017-10-30T16:36:58Z

I'll rebase against that tonight

PurpleBooth · 2017-10-31T07:19:15Z

Okay that's done now. (broken tests from master, again)

nicolas-grekas · 2017-12-08T19:47:19Z

Time to merge @symfony/deciders ?

xabbuh · 2017-12-11T07:58:40Z

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

@@ -21,6 +21,10 @@
 */
 class Email extends Constraint
 {
+    const VALIDATION_MODE_HTML5 = 'html5';
+    const VALIDATION_MODE_STRICT = 'strict';
+    const VALIDATION_MODE_LOOSE = 'loose';


What about being explicit now and markt these constants as public?

xabbuh · 2017-12-11T07:59:26Z

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

    public $message = 'This value is not a valid email address.';
    public $checkMX = false;
    public $checkHost = false;
+
+    /**
+     * @deprecated since version 3.4, to be removed in 4.0. Set mode to "strict" instead.


@deprecated since version 4.1, to be removed in 5.0. Set mode to "strict" instead.

The other messages need to be updated accordingly.

xabbuh · 2017-12-11T08:00:35Z

The deprecation also needs to be documented in UPGRADE-4.1.md, UPGRADE-5.0.md and the component's changelog file. But otherwise this looks good to me.

PurpleBooth · 2017-12-11T08:20:46Z

I'll update those tonight, or at lunchtime if I get a chance 👍

PurpleBooth · 2017-12-11T19:30:23Z

That should be the version numbers corrected in the deprication notices

Currently we only support a very loose validation. There is now a standard HTML5 element with matching regex. This will add the ability to set a `mode` on the email validator. The mode will change the validation that is applied to the field as a whole. These modes are: * loose: The pattern from previous Symfony versions (default) * strict: Strictly matching the RFC * html5: The regex used for the HTML5 Element Deprecates the `strict=true` parameter in favour of `mode='strict'`

fabpot

Looks great to me!

fabpot · 2017-12-11T20:06:05Z

Thank you @PurpleBooth.

This PR was merged into the 4.1-dev branch. Discussion ---------- [Validator] Html5 Email Validation Currently we only support a very loose validation. There is now a standard HTML5 element with matching regex. This will add the ability to set a `mode` on the email validator. The mode will change the validation that is applied to the field as a whole. These modes are: * loose: The pattern from previous Symfony versions (default) * strict: Strictly matching the RFC * html5: The regex used for the HTML5 Element Deprecates the `strict=true` parameter in favour of `mode='strict'` | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #21531 | License | MIT | Doc PR | symfony/symfony-docs#8487  Commits ------- cf04108 [Validator] Html5 Email Validation

This PR was merged into the master branch. Discussion ---------- [Validator] Html5 Email Validation Add documentation about the new mode parameter. Adds descriptions for the 'loose', 'strict', and 'html5' options. Relates to symfony/symfony#24442 Commits ------- 07d4bf9 [Validator] Html5 Email Validation

carsonbot added Status: Needs Review Validator Bug Feature BC Break Deprecation labels Oct 5, 2017

PurpleBooth mentioned this pull request Oct 5, 2017

[Validator] Add common double dot email typo #24429

Closed

PurpleBooth force-pushed the optional-email-filter branch from 2ce7aed to 5bc8189 Compare October 5, 2017 15:59

nicolas-grekas reviewed Oct 5, 2017

View reviewed changes

stof removed the Bug label Oct 5, 2017

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

stof requested changes Oct 5, 2017

View reviewed changes

stof reviewed Oct 5, 2017

View reviewed changes

stof requested changes Oct 5, 2017

View reviewed changes

stof added this to the 4.1 milestone Oct 5, 2017

PurpleBooth force-pushed the optional-email-filter branch 4 times, most recently from fa2c775 to 05f981c Compare October 5, 2017 16:50

PurpleBooth commented Oct 5, 2017

View reviewed changes

PurpleBooth force-pushed the optional-email-filter branch 9 times, most recently from 73b9c24 to a8df1a2 Compare October 5, 2017 17:16

PurpleBooth force-pushed the optional-email-filter branch 2 times, most recently from a7af07e to 8843696 Compare October 29, 2017 09:29

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 30, 2017

nicolas-grekas changed the base branch from 3.4 to master October 30, 2017 14:10

PurpleBooth force-pushed the optional-email-filter branch from 8843696 to 1752ba0 Compare October 30, 2017 23:29

PurpleBooth force-pushed the optional-email-filter branch from 1752ba0 to ae51161 Compare December 2, 2017 15:13

xabbuh reviewed Dec 11, 2017

View reviewed changes

PurpleBooth force-pushed the optional-email-filter branch 2 times, most recently from 879b4b4 to 9d5f021 Compare December 11, 2017 19:27

PurpleBooth force-pushed the optional-email-filter branch from 9d5f021 to 7796364 Compare December 11, 2017 19:38

PurpleBooth force-pushed the optional-email-filter branch from 7796364 to cf04108 Compare December 11, 2017 19:49

fabpot approved these changes Dec 11, 2017

View reviewed changes

fabpot merged commit cf04108 into symfony:master Dec 11, 2017

PurpleBooth deleted the optional-email-filter branch December 11, 2017 20:53

xabbuh mentioned this pull request Dec 13, 2017

[FrameworkBundle] add email_validation_mode option #25478

Merged

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Html5 Email Validation #24442

[Validator] Html5 Email Validation #24442

PurpleBooth commented Oct 5, 2017 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Oct 5, 2017

PurpleBooth Oct 5, 2017

stof Oct 5, 2017

nicolas-grekas Oct 5, 2017

nicolas-grekas Oct 5, 2017

nicolas-grekas Oct 5, 2017

nicolas-grekas Oct 5, 2017

stof left a comment

stof Oct 5, 2017

stof Oct 5, 2017

stof Oct 5, 2017

stof Oct 5, 2017

stof Oct 5, 2017

PurpleBooth Oct 5, 2017

nicolas-grekas Oct 5, 2017 •

edited

Loading

PurpleBooth commented Oct 29, 2017 •

edited

Loading

nicolas-grekas commented Oct 30, 2017

PurpleBooth commented Oct 30, 2017

PurpleBooth commented Oct 31, 2017

nicolas-grekas commented Dec 8, 2017

xabbuh Dec 11, 2017

xabbuh Dec 11, 2017

xabbuh Dec 11, 2017

xabbuh commented Dec 11, 2017

PurpleBooth commented Dec 11, 2017

PurpleBooth commented Dec 11, 2017

fabpot left a comment

fabpot commented Dec 11, 2017


		public function __construct($strict = false)
		public function __construct($mode = 'html5')


		return;
		} elseif (!interface_exists(EmailValidation::class) && !$strictValidator->isValid($value, false, true)) {

[Validator] Html5 Email Validation #24442

[Validator] Html5 Email Validation #24442

Conversation

PurpleBooth commented Oct 5, 2017 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

PurpleBooth commented Oct 29, 2017 • edited Loading

nicolas-grekas commented Oct 30, 2017

PurpleBooth commented Oct 30, 2017

PurpleBooth commented Oct 31, 2017

nicolas-grekas commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Dec 11, 2017

PurpleBooth commented Dec 11, 2017

PurpleBooth commented Dec 11, 2017

fabpot left a comment

Choose a reason for hiding this comment

fabpot commented Dec 11, 2017

PurpleBooth commented Oct 5, 2017 •

edited

Loading

nicolas-grekas Oct 5, 2017 •

edited

Loading

PurpleBooth commented Oct 29, 2017 •

edited

Loading