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

Core validation does not support new TLD domains #652

Closed
hissy opened this issue Aug 8, 2014 · 14 comments
Closed

Core validation does not support new TLD domains #652

hissy opened this issue Aug 8, 2014 · 14 comments
Milestone

Comments

@hissy
Copy link
Contributor

hissy commented Aug 8, 2014

Concrete\Core\Utility\Service\Validation\Strings::email() allows max 7 characters to TLD, but it does not support new longer TLDs.

eg: .international (13 characters)
http://www.newtldlist.com/

"How long can a TLD possibly be?"
http://stackoverflow.com/questions/9238640/how-long-can-a-tld-possibly-be

"Each node has a label, which is zero to 63 octets in length."
http://tools.ietf.org/html/rfc1034

In addition, there are (maybe unfortunately) multibyte TLDs.

.みんな
.公司
.닷컴

Wow... "A first step toward more global email"
http://googleblog.blogspot.jp/2014/08/a-first-step-toward-more-global-email.html

@hissy
Copy link
Contributor Author

hissy commented Aug 8, 2014

@mlocati
Copy link
Contributor

mlocati commented Aug 8, 2014

IMHO we should use a better approach to verify email addresses... What about is_email?

@aembler
Copy link
Member

aembler commented Aug 11, 2014

@mlocati That looks like a nice script. We'd want to wrap that functionality inside our existing email() function but it looks like a much better solution to validating email addresses. There also might be a nice object-oriented approach to this that we could get through composer and wrap inside our existing API.

@aembler aembler added this to the 5.7.0 Beta 2 milestone Aug 11, 2014
@mlocati
Copy link
Contributor

mlocati commented Aug 11, 2014

@hissy Do you know if is_email is ok for multibyte TLDs?

@aembler
Copy link
Member

aembler commented Aug 11, 2014

What about something like this? https://packagist.org/packages/egulias/email-validator

Basically is_email + OO approach

@joe-meyer
Copy link
Contributor

So... just toyed around with the egulias package. It seems to work ok, until I turn on the DNS check stuff. As soon as I flip that on (and put it in strict mode since that's the only way to check it actually uses it), I end up getting results saying there is no DNS record for the international domains.

If I ping the Bücher.ch example, it actually comes back and resolves to: xn--bcher-kva.ch. From what I can see his domain parser isn't doing any sort of magic to try and convert the domain to something like that so the checkdnsrr() function is failing.

Here's an output of some of my quick testing against the my branch https://github.com/ExchangeCore/concrete5-5.7.0/compare/fixes-652?expand=1. Let me know if you think this would be worthwhile to pull or if we need to find something else.

With DNS Validation

test@exchangecore.com : true
test@Bücher.ch : false
test@ü.ch : false
whoknowsif!works@exchangecore.com : true
thisisnotanemail : false
thisIsAlsoNotAnEmail@ : false
thisIs@Email : false
thisIsAValid@pa : false
test@католик : false
something@武@メール.グーグル : false
somethingInvalid@みんなみんなみんな : false

Without DNS Validation

test@exchangecore.com : true
test@Bücher.ch : true
test@ü.ch : true
whoknowsif!works@exchangecore.com : true
thisisnotanemail : false
thisIsAlsoNotAnEmail@ : false
thisIs@Email : true
thisIsAValid@pa : true
test@католик : true
something@武@メール.グーグル : true
somethingInvalid@みんなみんなみんな : true

@joe-meyer
Copy link
Contributor

Ya looks like Yii2 ran into a similar/same problem: yiisoft/yii2#312.

Also another oddity I just noticed with this package is that @pa is a valid tld with an mx record and it's still kicking back false for the check. Not that to many people are going to use a tld for their email (maybe?)

@aembler
Copy link
Member

aembler commented Aug 13, 2014

I would think we wouldn't enable DNS validation or mx record checking unless the developer really explicitly wanted that. Too risky and prone to breakage

Sent from my iPhone

On Aug 13, 2014, at 7:58 AM, Joe Meyer notifications@github.com wrote:

So... just toyed around with the egulias package. It seems to work ok, until I turn on the DNS check stuff. As soon as I flip that on (and put it in strict mode since that's the only way to check it actually uses it), I end up getting results saying there is no DNS record for the international domains.

If I ping the Bücher.ch example, it actually comes back and resolves to: xn--bcher-kva.ch. From what I can see his domain parser isn't doing any sort of magic to try and convert the domain to something like that so the checkdnsrr() function is failing.

Here's an output of some of my quick testing against the my branch https://github.com/ExchangeCore/concrete5-5.7.0/compare/fixes-652?expand=1. Let me know if you think this would be worthwhile to pull or if we need to find something else.

With DNS Validation

test@exchangecore.com : true
test@Bücher.ch : false
test@ü.ch : false
whoknowsif!works@exchangecore.com : true
thisisnotanemail : false
thisIsAlsoNotAnEmail@ : false
thisIs@Email : false
thisIsAValid@pa : false
test@католик : false
something@武@メール.グーグル : false
somethingInvalid@みんなみんなみんな : false

Without DNS Validation

test@exchangecore.com : true
test@Bücher.ch : true
test@ü.ch : true
whoknowsif!works@exchangecore.com : true
thisisnotanemail : false
thisIsAlsoNotAnEmail@ : false
thisIs@Email : true
thisIsAValid@pa : true
test@католик : true
something@武@メール.グーグル : true
somethingInvalid@みんなみんなみんな : true


Reply to this email directly or view it on GitHub.

@joe-meyer
Copy link
Contributor

@aembler it's not enabled by default, but the option was there in the past, and while it seems to work for non international domains and non top level domains, I wasn't sure if that was "good enough". If you think it is I can put up a pull request and we can just wait until a) someone complains b) the package gets updated and fixes the issue.

At this point we certainly don't lose any functionality by implementing, but the rules for a "valid" email become a little more lenient than what was previously there since it allows for top level domains (I.E. no . character in the domain part of the email address).

@hissy
Copy link
Contributor Author

hissy commented Aug 15, 2014

@mlocati it seems is_email() do not support multibyte TLDs

@joe-meyer
Copy link
Contributor

Looks like PR #715 got merged. Any thoughts feedback there? Hopefully it helps some with the TLD support, though international support seemed to be a bit lacking, and without relying on the PHP intl extension, i'm not sure how to even start about a PR to fix these issues. Anyway, went as far as I can probably go with this without feedback or guidance. If anyone wants to provide those I'm happy to dig further.

@hissy
Copy link
Contributor Author

hissy commented Aug 19, 2014

I tested and it works fine with new TLDs (including multibyte domains).

@aembler
Copy link
Member

aembler commented Aug 19, 2014

Nice! That is good news.

@aembler aembler closed this as completed Aug 19, 2014
@aembler
Copy link
Member

aembler commented Aug 19, 2014

Thanks everybody.

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

No branches or pull requests

4 participants