Fix eMail address regex

[mirabilos]

These commits address a few shortcomings of the current eMail address regular expression used for form validation:

• ‘.’ may not begin or end a local-part (SMTP RFC)
• limit host part total length to 255, label length to 63 (DNS RFC)
• case-sensitive comparison (speed optimisation and reliability)

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[mirabilos]

I signed it!

(although this diff does not pass threshold of originality, and as such is not covered by copyright protection)

[Narretz]

@mirabilos CLA is required for all Google projects, unfortunately. I wouldn't require it personally.

Can you include links to the docs on which these changes are based?

And we also need tests for these changes in this file: https://github.com/angular/angular.js/blob/master/test/ng/directive/inputSpec.js#L2784

[mirabilos]

Martin Staffa dixit:

@mirabilos CLA is required for all Google projects, unfortunately. I
wouldn't require it personally.

Yeswell, I did sign it, and today I even went and made sure
the eMail address I used in the commits was registered, so
go on ;)

Can you include links to the docs on which these changes are based?

RFC 822/5321 (SMTP) and RFC 1035 (DNS) are linked from the
individual commit messages, shouldn’t that suffice?

And we also need tests for these changes in this file:
https://github.com/angular/angular.js/blob/master/test/ng/directive/inputSpec.js#L2784

When I read that I thought “oh nooooo…” at first, but that
looks reasonable. I’ll see I can throw some together that
cover eMail addresses in more depth.

Is adding them in a subsequent commit okay with you? I don’t
much like destroying the actual history by rebasing too much.

Thanks,

//mirabilos

tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

[googlebot]

CLAs look good, thanks!

[googlebot]

CLAs look good, thanks!

[gkalpak]

Ranges such as #-' or \/-9 make this much less readable imo.

[mirabilos]

Georgios Kalpakas dixit:

Ranges such as #-' or \/-9 make this much less readable imo.

OK, I’ll expand them. But then, there are people who can’t read
a regex no matter how legible it is, and others will know about
it…

bye,

//mirabilos

tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

[mirabilos]

@Narretz tests included, let’s wait for Travis and hope I didn’t add a typo somewhere by accident… and that expanding the character class ranges didn’t fuck it up either…

[mirabilos]

@Narretz haha “line is too long”, any idea how I can address this test?

Inhouse we always exclude checks such as this one from being run on the testsuite, only on the main code.

In this special case, the line is legitimately very long as it tests a very long string… which, I suppose, I could concatenate, but… any suggestions? It’s your project after all.

[gkalpak]

LGTM (assuming the tests are green). Now, the big question: Is it a BC ?

[mirabilos]

@gkalpak still waiting for the tests, I tried to get around the line length issue by concatenating, and had a pasto in them indeed (something I tested from the command-line but mispasted).

Now, the big question: what is a “BC”?

https://www.mirbsd.org/wtf.cgi?q=BC doesn’t seem to match in this context…

[mirabilos]

I just recherched the maximum length of a local-part, and it’s 64 (RFC 5321 §4.5.3.1.1) so I’ll add another change to limit (and check!) that, but I’ll wait for this set of tests to pass first.

[Narretz]

BC means Breaking Change, i.e. if the change will break something that worked before in someone's application. Since this tightens it in terms of the spec, I think it's not BC.

BTW, we are going to squash the commits into one before merging (but preserve the commit message contents)

[gkalpak]

Lol, sorry for the acronym. So, "BC" is a breaking change.

Regarding, RFC 5321 §4.5.3.1.1, it sounds more like a recommendation than a hard limit:

Every implementation MUST be able to receive objects of at least
these sizes. Objects larger than these sizes SHOULD be avoided when
possible. However, some Internet mail constructs such as encoded
X.400 addresses (RFC 2156 [35]) will often require larger objects.

It might be better to leave local-part unrestricted.

[mirabilos]

No need to be sorry about the acronyms – I actually do collect them.

I’d prefer to have the commits not squashed, as they contain detail relevant for each partial change, but… I guess it’s your project.

This is not a breaking change because ① it only invalidates some eMail addresses previously passing as valid which are not valid in SMTP, and ② your own documentation – https://docs.angularjs.org/api/ng/input/input%5Bemail%5D – suggests users with specific requirements to use their own checks anyway.

I’d not let things unrestricted because the regex used is a “reasonably sane default” for eMail addresses, which omits, for example, all obsolete forms, the quoted localpart form, and the literal addresses like user@[10.0.0.1] and user@[IPv6:::1] from the set of valid addresses. In short, it’s a regex for accepting “what users are most likely to use as internet-reachable eMail address on a website signup form”. Site-local use, e.g. non-SMTP MTAs (Exchange, Lotus, X.400 or something) have specific rules, but I think they’re outside the scope of this. As the Perl people say: 「How to validate a user supplied email address is a FAQ (see perlfaq9): the only sure way to see if a supplied email address is genuine is to send an email to it and see if the user recieves it.」

In case you’re curious: there’s actually an eMail address validating exactly what the RFC says, but it’s so large it doesn’t even fit on my (laptop) screen. I can dig it out if needed.

I think the use case for this expression is to catch common typos and mistakes.

[gkalpak]

I’d prefer to have the commits not squashed, as they contain detail relevant for each partial change

We prefer to squash the commit when merging (so our history is cleaner on a higher level), but keep the unsquashed commits on the PR, which we link to from the squashed commit message. So, all the context is preserved and is easy to get to from the final commit.

I’d not let things unrestricted because the regex used is a “reasonably sane default” for eMail addresses, which omits, for example, all obsolete forms, the quoted localpart form, and the literal addresses like user@[10.0.0.1] and user@[IPv6:::1] from the set of valid addresses.

Point taken 😃

[mirabilos]

OK.

Oh great, now we have a line length issue in the regex, out of all places. I’ll try to wrap this.

(On the topic of acronyms, if you have any that http://www.mirbsd.org/wtf.htm or https://f-droid.org/repository/browse/?fdid=de.naturalnet.mirwtfapp doesn’t know, send them to me privately.)

[Narretz]

Don't wrap it. Use

/* jshint maxlen:250 */
// regex line
/* jshint maxlen:200 */

Untested, but should work

[mirabilos]

Thanks, that indeed worked. I think we’re ready for review then.