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] more strict e-mail validation regex #14853

Merged
merged 1 commit into from Jun 23, 2015
Merged

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jun 3, 2015

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

@nicolas-grekas
Copy link
Member

👍
Btw, I'm wondering if we ever considered using the W3's definition of an email as specified in HTML5?
This is much simpler than the full RFC, and still is already enforced by browsers for those using the "email" input type:

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

@Tobion
Copy link
Member

Tobion commented Jun 3, 2015

You sure this is according to the standard? From what I read, the domain may be bracketed.
RFC 2822, section 3.4.1

domain = dot-atom / domain-literal / obs-domain
domain-literal = [CFWS] "[" *([FWS] dcontent) [FWS] "]" [CFWS]

@nicolas-grekas
Copy link
Member

@@ -91,6 +91,7 @@ public function getInvalidEmails()
array('example'),
array('example@'),
array('example@localhost'),
array('foo@example.com bar'),
Copy link
Contributor

Choose a reason for hiding this comment

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

http://en.wikipedia.org/wiki/Email_address

admin@mailserver1 (local domain name with no TLD)

I'm not sure about the RFC, but it seems that the simple test should allow the localhost example.

@xabbuh
Copy link
Member Author

xabbuh commented Jun 4, 2015

Btw, I'm wondering if we ever considered using the W3's definition of an email as specified in HTML5?

Why not? But that should imo be considered a new feature and therefore be introduced in Symfony 2.8.

@xabbuh
Copy link
Member Author

xabbuh commented Jun 4, 2015

Actually, @iltar is right about localhost being a valid domain-part. The HTML5 pattern would accept that. Does everybody agree with switching to that regular expression then?

@fabpot
Copy link
Member

fabpot commented Jun 4, 2015

We had a very lengthy discussion about this a few months ago, changing anything in the current regex would be a BC break.

@xabbuh
Copy link
Member Author

xabbuh commented Jun 4, 2015

@fabpot Do you mean even the change forbidding whitespaces in the domain part would be a BC break?

@fabpot
Copy link
Member

fabpot commented Jun 4, 2015

Not necessarily. I just wanted to say that it would be very easy to break BC if we change the current regex we agree upon a while ago.

@nicolas-grekas
Copy link
Member

I'd be -1 on making the current rx even more weak. i.e. removing the "contains dot" constraint. It has been fine for months.

@xabbuh
Copy link
Member Author

xabbuh commented Jun 4, 2015

I didn't mean to remove the check for the period in the domain part. I meant that the HTML5 pattern that you suggested also addresses the issue @iltar raised.

@xabbuh
Copy link
Member Author

xabbuh commented Jun 9, 2015

So, what do we do here now?

@nicolas-grekas
Copy link
Member

I think this one is conservative enough to be merges asis
ping @symfony/deciders

@Tobion
Copy link
Member

Tobion commented Jun 23, 2015

👍

@fabpot
Copy link
Member

fabpot commented Jun 23, 2015

Thank you @xabbuh.

@fabpot fabpot merged commit 6491033 into symfony:2.6 Jun 23, 2015
fabpot added a commit that referenced this pull request Jun 23, 2015
This PR was merged into the 2.6 branch.

Discussion
----------

[Validator] more strict e-mail validation regex

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #14848
| License       | MIT
| Doc PR        |

Commits
-------

6491033 [Validator] more strict e-mail validation regex
@xabbuh xabbuh deleted the issue-14848 branch June 23, 2015 17:43
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

6 participants