jeremykendall / php-domain-parser

Public Suffix List based domain parsing implemented in PHP
MIT License
1.16k stars 128 forks source link

Rules Public API enhancements #263

Closed nyamsprod closed 4 years ago

nyamsprod commented 4 years ago

Issue summary

The current public API returns the following

$manager = new Manager(new Cache(), new CurlHttpClient());
$rules = $manager->getRules();
$domain = $rules->resolve('ec2-52-79-190-177.ap-northeast-2.compute.amazonaws.com');
$domain->getRegistrableDomain(); // "ec2-52-79-190-177.ap-northeast-2.compute.amazonaws.com"

While this the intended solution following the PSL rules, for users who do not read fully the documentation the returned value is not the expected one see #262 or #260 .

Instead to get what they expect (ie, the registrable domain as defined by ICANN they should do the following)

$manager = new Manager(new Cache(), new CurlHttpClient());
$rules = $manager->getRules();
$domain = $rules->resolve('ec2-52-79-190-177.ap-northeast-2.compute.amazonaws.com', Rules::ICANN_DOMAINS);
$domain->getRegistrableDomain(); // "amazonaws.com"

I would like to propose the following new methods to the Rules class:

Those methods would be aliases to the current Rules::resolve() method without having to use or remember the correct flags.

The method naming is still up for debate but the goal is to improve the public API and to remove the ambiguity and multiple issues with the current use of the Rules::resolve method.

What do you think @ack202 , @McAnix , @jeremykendall, @Shardj

nyamsprod commented 4 years ago

Also I did not mention it but those methods could throw instead of returning null value domain on failure

ack202 commented 4 years ago

I think it's a clean solution and more understandable as before.

nyamsprod commented 4 years ago

@ack202 do you have any objections to the names used šŸ¤”. I was thinking of using resolvexxxDomain as an alternative to xxxResolve where xxx is one of the choices

McAnix commented 4 years ago

Agreed on the resolveXXXDomain as it provides more clarity on the nature of the function. Happy with the shortcut too, thank you. The initial issue was a misunderstanding in the documentation.

On 15 Mar 2020, at 10:40, ignace nyamagana butera notifications@github.com wrote:

@ack202 https://github.com/ack202 do you have any objections to the names used šŸ¤”. I was thinking of using resolvexxxDomain as an alternative to xxxResolve where xxx is one of the choices

ā€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jeremykendall/php-domain-parser/issues/263#issuecomment-599182689, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJM6ZYHOKGTH6KRQXEXT2TRHSH67ANCNFSM4LG5ZE6Q.

nyamsprod commented 4 years ago

are added and merge to the develop branch, they differ from Rules::resolve in the fact that: