Fix pending PHPStan errors in PR #233

[Ndiritu]

Q	A
Bug fix?	fix #235
New feature?	no
BC breaks?	no
Deprecations?	no
Documentation	if this is a new feature, link to pull request in https://github.com/php-http/documentation that adds relevant documentation
License	MIT

What's in this PR?

• Fixes PHPStan failures in #233 by adding appropriate generic types to Promise PHPDocs
• Removes previous PHPStan error suppressions that are resolved.

To Do

• [ ] pending review/release of https://github.com/php-http/promise/pull/30

[dbu]

lets ignore the cs errors here, they will be fixed in #233

[dbu]

i tagged https://github.com/php-http/promise/releases/tag/1.2.1 and restarted the phpstan build. there are still some failures :thinking:

[Ndiritu]

i tagged https://github.com/php-http/promise/releases/tag/1.2.1 and restarted the phpstan build. there are still some failures 🤔

Taking a look.

[Ndiritu]

@dbu looking into the failures related to the $onRejected callable.

PHPStan seems to have some limitations in recognizing that a ClientExceptionInterface is still a Throwable. Not recognizing the inheritance structure seems to only be an issue when using callable([type]): [type] syntax. (Looking for a relevant issue on the PHPStan repo to link here or creating one myself)

• I'm proposing that we ignore the ClientExceptionInterface errors in this lib.
• While testing the callable syntax I've outline the impact of expanding the parameter type for $onRejected to \Throwable for a developer who may have previously been expecting an \Exception and is using PHPStan.

Please see the link below.

https://phpstan.org/r/ab8a6122-a065-4f37-8b65-b66633d212bf

[dbu]

thanks a lot for looking into this!

that would be this interface, right? https://github.com/php-fig/http-client/blob/master/src/ClientExceptionInterface.php says it extends \Throwable. is it possible to narrow down to a more specific interface in this context?

if even we can't make it work correctly, we should revert the annotations on the promise. otherwise everybody has to ignore the errors and everybody will complain first.

have you been able to check with the phpstan authors if your example should work? they are really good at improving phpstan, if it makes sense they can maybe fix it soonish.

[ste93cry]

I reasoned a bit on the linked playground, and I think that PHPStan is right (btw, the same error is reported by Psalm). What's happenning is that we're declaring in the interface that $onRejected can accept any Throwable. However, in the implementation, we are narrowing the argument type to be a specific exception class: that's not possible because it would mean crashing the script as soon as something is passed to it that is neither an instance of that class nor a subclass, so the tool reports it correctly.

[Ndiritu]

Thank you for breaking this down @ste93cry! I get it now. Would it be correct to say that the ideal fix then is to update this lib's onRejected parameter types to \Throwable instead of ClientExceptionInterface? Which would not be a breaking change if I understand correctly

[Ndiritu]

Alternatively, reverting the onRejected PHPDoc for the Promise interface to callable|null to prevent developers having to make changes. And probably specifying \Throwable in a major rev of the Promise lib?

https://phpstan.org/r/4ea07ab8-ccd1-4354-8373-e14eb14231f4

[ste93cry]

Would it be correct to say that the ideal fix then is to update this lib's onRejected parameter types to \Throwable instead of ClientExceptionInterface? Which would not be a breaking change if I understand correctly

Well, technically speaking it is a breaking change. You can narrow the return type in a child class because the declaration of the method is compatible, but you cannot narrow the type of an argument in a child class, because that would mean that the implementation cannot handle something that the parent declares it can. For this reason, since adding the typehint would mean that people have to change their child classes to take it into account, the same is true when adding the typehint in the DocBlock and then using a static analysis tool.

then we can correctly model that the promise can only ever throw ClientExceptionInterface

Couldn't the plugin also throw a ServerExceptionInterface?

[dbu]

there is no ServerExceptionInterface in PSR-18, the client exception is short to mean "http client exception" and psr18 says that every exception thrown must be a client exception. the async client follows the same logic in https://github.com/php-http/httplug/blob/2.x/src/HttpAsyncClient.php (except that for BC it has https://github.com/php-http/httplug/blob/2.x/src/Exception.php)

imo the correct solution is to make the exception customizable in the promise as well and in async client customize the promise to use Http\Client\Exception as the phpdoc text already explains but could not technically specify.

[ste93cry]

I wonder whether it makes sense to insist on trying to typehint onRejected more than now: the static analysis tool cannot infer which exception is thrown from it, so it cannot validate anyway that the next promise in the chain will receive a ClientExceptionInterface.

[dbu]

i think for documentation purposes its good to be clear what kind of exception may be thrown / has to be handled.

[ste93cry]

I came up with this solution that seems to work fine 🎉Of course, the $onRejected typehint is not checked, but PHPStan does not complain at least. I think it's the best we can do with the current tools.

[dbu]

I played around in the phpstan playground and came up with this: https://phpstan.org/r/6ec99fad-fd67-4f64-ac0d-ba135e207700

The upside is that the async client can declare what type of exception the onRejected callback must expect, to match with the current text description in phpdoc and the implementations of adapters.

any thoughts on that? i plan to implement this soonish if no objections come up.