-
Notifications
You must be signed in to change notification settings - Fork 136
Conversation
{ | ||
$validator = new Hostname(Hostname::ALLOW_ALL); | ||
// Check UTF-8 TLD matching | ||
$valuesExpected = [ |
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.
Please refactor using data providers.
Also split the test in two tests, one when is valid and another one when is not
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.
When refactor please add a description for each case using the key of the array
['invalid foo' => 'xn--3-owe4au9mpa.xn--xkc2al3hye2a']
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.
Only for testAdditionalPunycodedTLDs
or also for testAdditionalUTF8TLDs
(merged)?
Example dataProvider (valid hostnames):
public function validTLDHostnames()
{
// @codingStandardsIgnoreStart
return [
'test123.онлайн' => 'test123.онлайн',
'test123.онлайн' => 'test123.xn--80asehdb',
'тест.рф' => 'тест.рф',
'тест.рф' => 'xn--e1aybc.xn--p1ai',
'туршилтын.мон' => 'туршилтын.мон',
'туршилтын.мон' => 'xn--h1aggjjdd5b4a.xn--l1acc',
];
// @codingStandardsIgnoreEnd
}
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.
I'll appreciate if you can do it testAdditionalUTF8TLDs as well.
What I miss with the descriptions I don't quickly understand why that case is significative.
return [
'UTF8 TLD' => 'test123.онлайн',
'Punyencoded TLD' => 'test123.xn--80asehdb',
...
];
Method
isValid
must return true for IDN top level domains.