Add email validator option

mailman-elixir / mailman

Mailman provides a clean way of defining mailers in your Elixir applications

https://github.com/mailman-elixir/mailman

Other

203 stars 73 forks source link

Add email validator option #49

Closed 00dingens closed 7 years ago

00dingens commented 8 years ago

The default email address validator does not support all valid email addresses. This commit supports the usage of a email validator argument.

00dingens commented 8 years ago

I'm sorry that my editor struggled with all the whitespace.

kamilc commented 8 years ago

I can see two problems with this pull request:

The new validator argument for the envelope_email function isn't being used
I'm not sure this part should really be configurable. Address validation with regexes is known not to be perfect in any case. We can provide the one that handles 99% cases or think about validating some other way.

Ideas?

kamilc commented 8 years ago

@00dingens Any thoughts?

mgumz commented 7 years ago

"I'm not sure this part should really be configurable. Address validation with regexes is known not to be perfect in any case. We can provide the one that handles 99% cases or think about validating some other way."

Exactly.

And the one in use right now (see https://github.com/kamilc/mailman/blob/master/lib/mailman/external_smtp_adapter.ex#L31 ) is not sufficient in "our" cases (eg, it does not allow a TLD such a .agency, .blackfriday or .guitars) since each project has their own needs, a configurable email-validator is something nice to have.

kamilc commented 7 years ago

@mgumz @00dingens I've just removed the validation we have at the smtp level and pushed. We'll figure out some configurable way of adding that back in. Thanks for your suggestions and time!