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

Replace email validator #214

Merged
merged 1 commit into from Jun 8, 2020
Merged

Replace email validator #214

merged 1 commit into from Jun 8, 2020

Conversation

thusoy
Copy link
Contributor

@thusoy thusoy commented Apr 10, 2020

This limits emails to "sane" emails which doesn't allow legacy features like quoted parts, comments, etc. This is more likely to be what people actually expect in an email address.

@ljharb
Copy link
Collaborator

ljharb commented May 24, 2020

Seems like the comment test is still failing.

@thusoy
Copy link
Contributor Author

thusoy commented May 25, 2020

Yes, it fails because you overwrote my changes to that test. The old validator was unable to validate comments, this one does. I modified the test to accomodate that, but you removed those changes.

@ljharb
Copy link
Collaborator

ljharb commented May 25, 2020

I'm confused. There's now a test on master (yours) that includes comments, that passes on master. It shouldn't require modification in this PR to pass here, but it fails here.

For example, here is one of yours: 0d7ec01#diff-b5c163a3676f9b1a4e6c960d07a2eeaeR174 and here's the same test pulled into master: https://github.com/caolan/forms/blob/master/test/test-validators.js#L174.

If I accidentally lost important changes, my apologies - could you restore them?

@thusoy
Copy link
Contributor Author

thusoy commented May 25, 2020

Yes, that test asserts that emails with comments shouldn't be allowed since the regex didn't validate them, but those addresses are RFC compliant, thus when swapping the regex for an RFC-compliant parser that test needed to have it's assertion reversed. I'll push that again.

@ljharb ljharb merged commit d4bd5b5 into caolan:master Jun 8, 2020
ljharb pushed a commit that referenced this pull request Jun 9, 2020
@thusoy thusoy deleted the email branch June 9, 2020 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants