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

SchemaRegEx.Email is not entirely valid #59

Closed
Nemo64 opened this issue Jan 25, 2014 · 6 comments
Closed

SchemaRegEx.Email is not entirely valid #59

Nemo64 opened this issue Jan 25, 2014 · 6 comments

Comments

@Nemo64
Copy link
Contributor

Nemo64 commented Jan 25, 2014

SchemaRegEx.Email is not correnct.

However this needs to be discussed. Technically the address root+something@localhost is a valid email and will be accepted by the browsers input type email. Maybe multiple pre made expressions are needed to fill what the developer needs.

I eg. like to allow the '+' as it allows the user (if he knows about that) to figure out where the mail came from.

Then again I dislike allowing domains that don't have a '.' as in the normal case we are not in an intranet and such mails aren't what we want. However for the intranet case this should be there too.

@aldeed
Copy link
Collaborator

aldeed commented Jan 26, 2014

It might be best to have a few different varieties built in, perhaps add an IntranetEmail expression but leave the current as is. At the time I was initially writing the built-in expressions, I know I researched and tested half a dozen different expressions and settled on this one as the most correct for most general uses. However, I don't remember much about my decision process anymore. I was probably thinking that 9 times out of 10 we don't want domains without TLDs. I might not have considered the use case for "+" at all. I'm definitely open to PRs for additional built-in expressions, or possibly we can adjust Email to allow the "+" but still require the TLD. Others are welcome to comment their thoughts.

@steph643
Copy link

I strongly advise to keep regex-based email validation as permissive as possible. anything@anything.anything is enough. The only way to validate an email address is to send an email.

@dwlf
Copy link

dwlf commented May 10, 2014

@steph643 what is your reasoning? The new top-level domains would seem to make pattern matches a little more difficult, though it is still in the best interest of UX to try & detect issues and notify the user.

Also, it isn't like this isn't a solved problem. I'd look to projects like WordPress to see what pattern matching it employs.

@aldeed I would like to see '+' supported if it isn't currently.

@steph643
Copy link

There are many discussions about this on the Internet. Google: regex email validation.
From an API provider perspective, I think the safest way (unless you want to spend endless hours in support and debates) is to only check anything@anything.anything, then to allow users to provide an alternate algorithm.

@aldeed
Copy link
Collaborator

aldeed commented May 11, 2014

My goal really is just to give a simple built-in option for people who want to get up and running quickly. You can, of course, provide any regex you find online or create yourself rather than using the built-in. You can also reset SimpleSchema.RegEx.Email to any regex you wish. So I don't think the aim is to please everyone.

On the other hand, if a package like this can provide a few different expressions that meet the most common validation needs, then that potentially saves a lot of time for a lot of people who would otherwise have to research and find/write their own.

So I still think it might be best to provide a few different options. Maybe the default should indeed be the most permissive, and then one or two less permissive alternatives can be available. Regarding "anything@anything.anything", one could argue that "anything@anything" (i.e., contains one "@" that is not the first character or the last character) is more accurately the bare minimum because, according to the spec, the domain could be an IP address or "localhost".

@aldeed
Copy link
Collaborator

aldeed commented May 11, 2014

Didn't see PR #89 until after responding. I think that PR matches pretty well with what I said. Anyone else have comments on it?

@aldeed aldeed closed this as completed in effdb04 May 13, 2014
aldeed added a commit that referenced this issue May 13, 2014
resolves #59 and adds more precise expressions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants