Closed lyrixx closed 3 years ago
@lyrixx thanks for using the library.
Do you get an actual PHP error or is it that you think that there's an error. What you just described is called parameter widening and it is supported in PHP since PHP7.2 if I'm not mistaking. Unless I fail to understand your issue in which case a code snippet would help me understand it.
Indeed, there are no PHP error, but linter (vscode, phpstan) fails
strange that phpstan fail to that since this package uses PHPStan as one of it's static analysis tool and it never reacted to that 🤔 . Anyway, this is an expected behaviour since PHP7.2 so there's nothing I can do 🤷♂️
so there's nothing I can do
Actually, you can :) You can change the typehint of the PsRStorageFactory :)
@lyrixx Why would I do that ? It returns Rules
which is an implementation of the PublicSuffixList
interface. IMHO It should not typehint on a specific implementation ? When using the factory you should only care about the interface never about any specific implementation. Unless you consider the factory to be specific to the Rules 🤔
Also the only class that exposes the implementation details is PublicSuffixListPsr18Client
.
Every other classes relies solely on the interface which I believe is the correct thing to do
Actually, I don't really know. I manage to make it work by using a Domain
instance instead of a string.
But this was mentioned nowhere in the README that we could do like this.
May be a sentence in the README would be enough to fix this design issue (no offence!!).
I said design issue, because a lib should not leverage the contravariance rule internally ; this rule existe only for librairie wanting to update there signature to add typing without breaking code that use it. This is not the case here.
I need a code snippet to understand what we are talking about because I'm a bit lost. Is your issue around the storage services or around how the PublicSuffixList/Rules works ?
In my mind:
PublicSuffixList
you should feed resolve
with a Host
objectRules
you can feed resolve
with anything that may be accepted like it is explained in the README fileThe Storage Facility should always be used with Interface and the fact that PublicSuffixListPsr18Client
happens to return a Rules
object is just an implementation detail.
According to the README, I could write:
$result = $publicSuffixList->resolve('www.PreF.OkiNawA.jP');
And I have the following class:
<?php
use Pdp\Domain;
use Pdp\Storage\PsrStorageFactory;
class DomainChecker
{
private PsrStorageFactory $factory;
public function __construct(PsrStorageFactory $factory)
{
$this->factory = $factory;
}
public function isSameDomain(string $firstDomain, string $newDomain): bool
{
$storage = $this->factory->createPublicSuffixListStorage('pdp_', new \DateInterval('P1D'));
$publicSuffixList = $storage->get(PsrStorageFactory::PUBLIC_SUFFIX_LIST_URI);
$currentRegistrableDomain = $publicSuffixList->resolve(Domain::fromIDNA2008($firstDomain))->registrableDomain()->toString();
$newRegistrableDomain = $publicSuffixList->resolve(Domain::fromIDNA2008($newDomain))->registrableDomain()->toString();
return $currentRegistrableDomain === $newRegistrableDomain;
}
}
As you can see I had to write
Domain::fromIDNA2008($firstDomain)
Instead of
$firstDomain
(BTW: Am I using your library well?)
My point is:
Rules
and its interface PublicSuffixList
. I know you are using "Parameter Type Widening" feature of PHP. But IMHO this is a wrong usage of this feature, since its primary goal is for adding a parameter in a library without breaking the BC.I may be wrong about 2/, but this is what is stated by the RFC:
Implementing this RFC would allow libraries to be upgraded to use type declarations in their method signatures. Currently adding types to a method of a class in a library would break any code that extends that class.
This would provide an easier upgrade path for libraries to start using scalar types, to replace manual checks being done inside the methods, without requiring an update for all sub-classes.
Here, it seems you are using this feature internally, to allow more input type in the resolve()
method, for ease the usage of the lib (which is super nice).
Anyway, thanks a lot for taking time to discuss about this issue which is not really important :) Feel free to close it, if it's OK for you.
But, here is a list of options to address this issue if you want to:
Rules
classresolveString(string $host)
) in the Rules
class that accept only a string. This method will use the resolve()
internalyOk now I understand your concern so,
in your example what I would do is this
<?php
use Pdp\Domain;
use Pdp\Storage\PublicSuffixList;
class DomainChecker
{
private PublicSuffixList $publicSuffixList;
public function __construct(PublicSuffixList $publicSuffixList)
{
$this->publicSuffixList = $publicSuffixList;
}
public function isSameDomain(string $firstDomain, string $newDomain): bool
{
$currentRegistrableDomain = $this->publicSuffixList->resolve(Domain::fromIDNA2008($firstDomain))->registrableDomain()->value();
$newRegistrableDomain = $this->publicSuffixList->resolve(Domain::fromIDNA2008($newDomain))->registrableDomain()->value();
return $currentRegistrableDomain === $newRegistrableDomain;
}
}
And the PsrStorageFactory
would never appear in the code only in the DI configuration file.
Since you are typehinting with the Interface I find it OK to call Domain::fromIDNA2008
since that's how it should be used with the interface.
Now If you typehint against the Rules
implementation I think that a smart DI container will do the resolution for you but then again in the README I do write the following:
TIP: Always favor submitting a Pdp\Domain object for resolution rather that a string or an object that can be cast to a string to avoid unexpected format conversion errors/results. By default, and with lack of information conversion is done using IDNA 2008 rules.
Which I think covers your use case. Granted this is written when talking about domain i18n conversion but it is written in bold.
Last but not least, note that in your case I use Domain::value
instead of Domain::toString
simply because it is more strict and avoid any typecasting issue.
Hope this clarify your issue... or I'm off topic.
Anyway, thanks a lot for taking time to discuss about this issue which is not really important :)
When designing version 6.0 I wanted to make it super DX-friendly so I do take into account remarks when people do actually use the lib and submit constructive questions/enquiries. If it can be improve let's improve it with interesting feedbacks like your ;)
OK, so to resume this issue:
I should always use the a Domain::fromIDNA2008()
and not a raw string :)
Thanks for your help. We can close the issue
@lyrixx forgot to mention... I would call either the toAscii
or toUnicode
method before calling the value
method just to be sure that both domains are is the same representative mode
One last thing... Did you happen to use the lib in a previous release? And how do you rate the DX ? That's my curiosity speaking
Yes, I used the lib in the previous version, the DX was really simple.
And TBH, I was a bit lost during the upgrade (mainly because of this issue). I read the README, and tell myself, ok it should be quite easy. But then, after the cache was setup, I got the mismatch typehint.
Oh, and another thing: I the readme you talk about "The Public Suffix List" and "The IANA Top Level Domain List" but it's not obvious what is it. Maybe add a description for each + a sentence to help the dev to make a choice
I almost forgot, you lost me also with: $publicSuffixList = Rules::fromPath('/path/to/cache/public-suffix-list.dat');
=> where does this list come from? when reading the whole readme, we understand how to get that with a cache. But I guess you know how developper are: lazy :p They like to copy paste code :) So I would rework a bit the readme with file_get_contents(...)
. It'll show even more the need for a cache layer
Thanks a lot for this discussion 👍🏼
Hello, I think there is something wrong with the typing:
Issue summary
It might not be a big deal, but when we use the
PsRStorageFactory
:and
PublicSuffixListStorage
:So, since we don't use
Rules
directly but an interfaces, there are an error when calling with a string as stated in the README.System informations