Skip to content
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] In "strict" mode, email validator inaccurately claims certain valid emails are invalid #18157

Closed

Conversation

natechicago
Copy link
Contributor

Q A
Branch 2.5
Bug fix? yes
New feature? no
BC breaks? potentially
Deprecations? no
Tests pass? yes
Fixed tickets #18156
License MIT
Doc PR n/a

@patrick-mcdougle
Copy link
Contributor

Should the second argument pass the value of $constraint->checkMX as the second argument? If so, should we return out of validate if it comes back as valid?

@natechicago
Copy link
Contributor Author

@patrick-mcdougle Interesting question/suggestion. In general, I'd probably recommend not including such a change in this particular PR, since it's out of scope relative to the bug this PR is trying to remedy. But even if it had its own PR, I don't think we'd gain any benefit from sending that info to the external email validation library. The MX record check performed by the external library is basically identical to the MX record check that Symfony is already well-equipped to handle in its own email validator.

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

$strictValidator = new \Egulias\EmailValidator\EmailValidator();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent here is to send the 3rd argument as false, rather than true. But thanks to the default values for the 2nd and 3rd arguments (false and false, respectively), they can now be omitted completely.

@nicolas-grekas
Copy link
Member

I think this should be considered as a new feature, and we should allow the user to decide which mode is appropriated, with a default to the same as today and a new html5 mode also (see #18156 (comment))

@natechicago
Copy link
Contributor Author

Closing this PR in favor of #18428.

@natechicago natechicago closed this Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants