-
-
Notifications
You must be signed in to change notification settings - Fork 215
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
Fix labels length check #278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vajexal, thanks for your PR.
I've left some comment for you.
src/Parser/DomainPart.php
Outdated
private function isLabelTooLong($label) | ||
{ | ||
if (preg_match('/[^\x00-\x7F]/', $label)) { | ||
$label = idn_to_ascii($label, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46, $idnaInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are overriding the original $label
from the parameters, please use another variable, i.e $asciiLabel
.
src/Parser/DomainPart.php
Outdated
if (preg_match('/[^\x00-\x7F]/', $label)) { | ||
$label = idn_to_ascii($label, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46, $idnaInfo); | ||
|
||
if (!$label) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could remove the if
and directly return return (bool) ($idnaInfo['errors'] | IDNA_ERROR_LABEL_TOO_LONG)
, label has passed the length test already so testing again in line 422 wouldn't be needed.
Good job @vajexal ! Thanks for contributing. |
'example@toolonglocalparttoolonglocalparttoolonglocalparttoolonglocalparttoolonglocalparttoolonglocal'. | ||
'parttoolonglocalparttoolonglocalparttoolonglocalparttoolonglocalparttoolonglocalparttoolonglocalpart'. | ||
'toolonglocalparttoolonglocalparttoolonglocalparttoolonglocalpar' | ||
], | ||
[ | ||
[LabelTooLong::CODE,], | ||
sprintf('example@%s.com', str_repeat('ъ', 60)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vajexal while porting to v3, why this should rise the warning? it is a 60 char label long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because if we convert label to idn, it's length will be > 63
var_dump(idn_to_ascii(str_repeat('ъ', 60), IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46, $idnaInfo), $idnaInfo);
// bool(false)
// array(3) {
// ["result"]=> string(66) "xn--z1aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
// ["isTransitionalDifferent"]=> bool(false)
// ["errors"]=> int(2)
// }
Merging in order to release v3. Can you answer the question anyway please? |
checkLabelLength
method because lexer parses digits and unicode chars as different tokens