fix #234

[Insolita]

Introduction

So, in most cases only Domain VO options enough; There are no way to configure it via PublicSuffix, because it use idn-convertion functions before set PublicSuffix https://github.com/jeremykendall/php-domain-parser/blob/develop/src/Domain.php#L102

PublicSuffix VO also has ability to be configured with custom options, for cases when user want to set some specific PublicSuffix;

Backward Incompatible Changes

No

Open issues

https://github.com/jeremykendall/php-domain-parser/issues/234

[nyamsprod]

@Insolita you should not PR against the master branch but against the develop branch instead.

Also you PR seems to be impacting domain resolution which follow a specific algorithm. Why not just add a ::format method with two parameters the mode (ASCII or UTF8) and the IDNA options :question:

[Insolita]

hm. probably https://github.com/jeremykendall/php-domain-parser/blob/develop/.github/CONTRIBUTING.md is not clear about branches, with bold "Always try the master branch "

It didn'nt any influence on result with default arguments. That is matter - if u have deals with domains that contains some specific characters, by default it can resolve two different domain names as same. As a way, we should pass boolean flag into constructor - like 'supportNonTransitional' and apply concrete options for keep it; Additional format method can't solve the problem, important symbols lossed after splitting into labels;

[nyamsprod]

After reading again your issue I believe this class from URL parser may be of interest since your issue is idn to ASCII conversion. Can this be a basis to improve your PR ? https://github.com/TRowbotham/URL-Parser/blob/master/src/IDN.php .

[Insolita]

Yes, this class is near, and they append valid IDNA_NONTRANSITIONAL_TO_ASCII option by default, https://github.com/TRowbotham/URL-Parser/blob/master/src/IDN.php#L98 But it only for ascii, same should be also for unicode conversion;
You want move IDNAConverterTrait into separated class with ability to pass it into Domain/PublicSuffix constructor instead of options? The main problem is that convertation happens into Domain constructor immediately, and the library has many methods where Domain instance created internally (and PublicSuffix also), so we have many places where custom options should be passed.

[nyamsprod]

I'm thinking adding a clear IDN class which is called by the current trait or which completely replace it. Since all classes are private there should be no BC break unless we the changes are too important 🤔

[nyamsprod]

Also how does these changes affect domain resolution. Do we also need to adjust the Converter and the Rules classes ?

[nyamsprod]

hm. probably https://github.com/jeremykendall/php-domain-parser/blob/develop/.github/CONTRIBUTING.md is not clear about branches, with bold "Always try the master branch "

This means you should try the master branch to see if the submitted bug is not already fix it does not imply that the PR must be made against the master branch :smile: . I will update this part since it is misleading.