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

isEmail should check specific gmail case only under option #853

Closed
towc opened this issue Jun 20, 2018 · 10 comments · Fixed by #873
Closed

isEmail should check specific gmail case only under option #853

towc opened this issue Jun 20, 2018 · 10 comments · Fixed by #873

Comments

@towc
Copy link
Contributor

towc commented Jun 20, 2018

I think it's weird to only have some rules about gmail, which might require more maintenance, create bugs, and in my case, a bit of confusion.
I was writing some tests, and one of them used the email "[email protected]", but the test didn't pass. It took me a bit to figure out that the problem was actually with validator not allowing gmail usernames to be outside of the 6-30 range length.
It's still nice to have some additional checks, but maybe I'd only put them under an option? validator.isEmail(email, { domainChecks: true }) or similar.

Agree or disagree? Would it make anyone's code incompatible or work worse, if it's not the default?

@tux-tn
Copy link
Member

tux-tn commented Jun 23, 2018

Hello towc

I am the person who added gmail validation to isEmail.
I disagree with you on this, [email protected] is an invalid email address (you can't send an email to [email protected] or create an address with [email protected]), why would you want your test to pass with an invalid email address?

@towc
Copy link
Contributor Author

towc commented Jun 23, 2018

as mentioned:

I think it's weird to only have some rules about gmail, which might require more maintenance, create bugs, and in my case, a bit of confusion.

And if the idea is to allow gmail, why stop there and not add other common providers? If you actually want specific validation, it means that the code that uses validator would need separate cases for the other common providers, and then it will be confusing because you won't see gmail in there unless you want to duplicate validations unnecessarily, which means now there are unnecessary comments. And if you did want the providers to be validated by this package, then we should add more of them, under an option anyway.

I'm not trying to undervalue your contribution, but there were various commits because the validation didn't account for some stuff, until someone mentioned it. It's not that you didn't do your research, but it shows a deeper problem with maintaining these things. I don't think it's this library's place to do these kinds of checks by default. Or at the very least we could have an option to skip them, so the default remains the same, but it still makes it unintuitive and cause for confusion

@tux-tn
Copy link
Member

tux-tn commented Jun 23, 2018

I think you'll understand better why gmail validation has been added to isEmail by taking a look at #825

@towc
Copy link
Contributor Author

towc commented Jun 23, 2018

oh, thanks, I hadn't read that. Either way, normalizeEmail uses options. I think we should also use options in isEmail

@xlchurch
Copy link

ran into the same issue as @towc.

I think it make's sense to have provider specific validation enable by default but it would be nice to pass options to disable the additional validation for all providers or specific providers if required.

@xlchurch
Copy link

even updating the isEmail description in the Validators list to detail what validation is occurring for each provider would be a step in the right direction... opposed to finding this out during implementation.

@twelve17
Copy link

twelve17 commented Sep 5, 2018

@tux-tn it’s possible there are some gmail users who have addresses that were valid at the time that wouldn’t be allowed today. Perhaps the more strict validation was added later, but we are still left with the possibility that these emails exist legitimately. See my comment here for more info
#825 (comment)

I hope an option can be added to disable these kind of domain specific checks, otherwise I will have to add logic to optionally do it outside of this library.

@tux-tn
Copy link
Member

tux-tn commented Sep 5, 2018

@twelve17 check #873 the flag domain_specific_validation has been added

@twelve17
Copy link

twelve17 commented Sep 5, 2018

@tux-tn ah, thank you! Sorry for the trouble.

@suhailgupta03
Copy link

@tux-tn Why was this removed from the subsequent versions? For example 10.11.0 does not suffer from this but 10.4.0 does.

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 a pull request may close this issue.

5 participants