New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Validator] Html5 Email Validation #24442

Merged
merged 1 commit into from Dec 11, 2017

Conversation

@PurpleBooth
Contributor

PurpleBooth commented Oct 5, 2017

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)

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

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

This comment has been minimized.

@PurpleBooth

PurpleBooth Oct 5, 2017

Contributor

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;
        }
    }

This comment has been minimized.

@stof

stof Oct 5, 2017

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

should be revert, to ease reviewing the patch

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

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

@stof stof removed the Bug label Oct 5, 2017

@stof

Note having any test for the new mode is wrong.

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

This comment has been minimized.

@stof

stof Oct 5, 2017

Member

should be removed.

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

This comment has been minimized.

@stof

stof Oct 5, 2017

Member

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

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

This comment has been minimized.

@stof

stof Oct 5, 2017

Member

missing negation too

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

This comment has been minimized.

@stof

stof Oct 5, 2017

Member

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)

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

This comment has been minimized.

@stof

stof Oct 5, 2017

Member

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)

@stof stof added this to the 4.1 milestone Oct 5, 2017

@PurpleBooth PurpleBooth force-pushed the PurpleBooth:optional-email-filter branch 4 times, most recently from fa2c775 to 05f981c Oct 5, 2017

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

This comment has been minimized.

@PurpleBooth

PurpleBooth Oct 5, 2017

Contributor

Not terribly happy with this. Suggestions?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

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 PurpleBooth force-pushed the PurpleBooth:optional-email-filter branch 9 times, most recently from 73b9c24 to a8df1a2 Oct 5, 2017

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

@PurpleBooth

This comment has been minimized.

Contributor

PurpleBooth commented Oct 29, 2017

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

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 30, 2017

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

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

@nicolas-grekas nicolas-grekas changed the base branch from 3.4 to master Oct 30, 2017

@PurpleBooth

This comment has been minimized.

Contributor

PurpleBooth commented Oct 30, 2017

I'll rebase against that tonight

@PurpleBooth PurpleBooth force-pushed the PurpleBooth:optional-email-filter branch from 8843696 to 1752ba0 Oct 30, 2017

@PurpleBooth

This comment has been minimized.

Contributor

PurpleBooth commented Oct 31, 2017

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

@PurpleBooth PurpleBooth force-pushed the PurpleBooth:optional-email-filter branch from 1752ba0 to ae51161 Dec 2, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 8, 2017

Time to merge @symfony/deciders ?

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

This comment has been minimized.

@xabbuh

xabbuh Dec 11, 2017

Member

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

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.

This comment has been minimized.

@xabbuh

xabbuh Dec 11, 2017

Member

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

This comment has been minimized.

@xabbuh

xabbuh Dec 11, 2017

Member

The other messages need to be updated accordingly.

@xabbuh

This comment has been minimized.

Member

xabbuh commented Dec 11, 2017

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

This comment has been minimized.

Contributor

PurpleBooth commented Dec 11, 2017

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

@PurpleBooth PurpleBooth force-pushed the PurpleBooth:optional-email-filter branch 2 times, most recently from 879b4b4 to 9d5f021 Dec 11, 2017

@PurpleBooth

This comment has been minimized.

Contributor

PurpleBooth commented Dec 11, 2017

That should be the version numbers corrected in the deprication notices

@PurpleBooth PurpleBooth force-pushed the PurpleBooth:optional-email-filter branch from 9d5f021 to 7796364 Dec 11, 2017

[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'`

@PurpleBooth PurpleBooth force-pushed the PurpleBooth:optional-email-filter branch from 7796364 to cf04108 Dec 11, 2017

@fabpot

fabpot approved these changes Dec 11, 2017

Looks great to me!

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 11, 2017

Thank you @PurpleBooth.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 11, 2017

feature #24442 [Validator] Html5 Email Validation (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

<!--
- Bug fixes must be submitted against the lowest branch where they apply
  (lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the 3.4,
  legacy code removals go to the master branch.
- Please fill in this template according to the PR you're about to submit.
- Replace this comment by a description of what your PR is solving.
-->

Commits
-------

cf04108 [Validator] Html5 Email Validation

@PurpleBooth PurpleBooth deleted the PurpleBooth:optional-email-filter branch Dec 11, 2017

xabbuh added a commit to symfony/symfony-docs that referenced this pull request Dec 22, 2017

minor #8487 [Validator] Html5 Email Validation (PurpleBooth)
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

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment