Fix pending PHPStan errors in PR #233 #234

Ndiritu · 2023-11-07T19:59:34Z

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 Accept Throwable for onRejected and expand return type for wait() promise#30

dbu

thanks!

dbu · 2023-11-07T20:57:44Z

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

dbu · 2023-11-08T13:00:21Z

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

Ndiritu · 2023-11-08T13:16:47Z

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 · 2023-11-15T16:16:01Z

@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 · 2023-11-22T13:00:45Z

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 · 2023-11-23T08:15:15Z

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 · 2023-11-23T10:22:03Z

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 · 2023-11-23T10:32:57Z

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

dbu

indeed https://github.com/php-http/promise/blob/7fa2284a91241e7fa06976369e998933d510da1b/src/Promise.php#L42C1-L42C88 is a BC break (in terms of phpdoc). we can't narrow down what type of exception the callable must accept. i think the correct thing would be to have a second covariant for the exception of the promise. then we can correctly model that the promise can only ever throw ClientExceptionInterface

can you try that?

and we should adjust the HttpAsyncClient in the httplug repository to declare what promise it returns, then we don't need to change all the various async client implementations here.

dbu · 2023-11-23T10:37:26Z

src/Deferred.php

@@ -122,6 +130,9 @@ public function reject(ClientExceptionInterface $exception): void
        }
    }

+    /**
+     * {@inheritDoc}
+    */


we don't do inheritdoc when there is nothing else. if there is no docblock, most tools take the docblock of the parent/interface

dbu · 2023-11-23T10:37:51Z

src/Deferred.php

@@ -90,6 +97,7 @@ public function getState(): string

    /**
     * Resolve this deferred with a Response.
+     * @param ResponseInterface $response


we don't add phpdoc when it does not add information

Suggested change

* @param ResponseInterface $response

dbu · 2023-11-23T10:38:45Z

src/Deferred.php

@@ -51,6 +53,11 @@ public function __construct(callable $waitCallback)
        $this->onRejectedCallbacks = [];
    }

+    /**
+     * @param callable(ResponseInterface): ResponseInterface|null $onFulfilled


this should not be necessary with the @implements in the class docblock, it should come automatically from the covariant in the interface.

dbu · 2023-11-23T10:40:08Z

src/HttpAsyncClientEmulator.php

@@ -23,6 +23,7 @@ abstract public function sendRequest(RequestInterface $request): ResponseInterfa

    /**
     * @see HttpAsyncClient::sendAsyncRequest
+     * @return \Http\Promise\Promise<ResponseInterface|\Throwable>


add in interface instead

dbu · 2023-11-23T10:41:19Z

src/HttpClientPool/HttpClientPool.php

@@ -52,6 +53,9 @@ public function addHttpClient($client): void
     */
    abstract protected function chooseHttpClient(): HttpClientPoolItem;

+    /**
+     * @return Promise<ResponseInterface>


not needed when HttpAsyncClient defines the return type

dbu · 2023-11-23T10:41:46Z

src/HttpClientPool/HttpClientPoolItem.php

@@ -89,6 +90,9 @@ public function sendRequest(RequestInterface $request): ResponseInterface
        return $response;
    }

+    /**
+     * @return Promise<ResponseInterface>


not needed when HttpAsyncClient defines the return type

dbu · 2023-11-23T10:42:02Z

src/HttpClientRouter.php

@@ -26,6 +27,9 @@ public function sendRequest(RequestInterface $request): ResponseInterface
        return $this->chooseHttpClient($request)->sendRequest($request);
    }

+    /**
+     * @return Promise<ResponseInterface>


not needed when HttpAsyncClient defines the return type

dbu · 2023-11-23T10:43:36Z

src/PluginClient.php

@@ -84,6 +84,9 @@ public function sendRequest(RequestInterface $request): ResponseInterface
        return $pluginChain($request)->wait();
    }

+    /**
+     * @return Promise<ResponseInterface>


not needed when HttpAsyncClient defines the return type

dbu · 2023-11-23T10:45:41Z

src/HttpAsyncClientDecorator.php

@@ -21,6 +23,7 @@ trait HttpAsyncClientDecorator

    /**
     * @see HttpAsyncClient::sendAsyncRequest
+     * @return Promise<ResponseInterface>


we need to fix this in the HttpAsyncClient instead. then it will be inferred for all the various implementations here:

https://github.com/php-http/httplug/blob/625ad742c360c8ac580fcc647a1541d29e257f67/src/HttpAsyncClient.php#L20

ste93cry · 2023-11-23T23:37:40Z

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 · 2023-11-24T12:51:14Z

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 · 2023-11-25T01:08:46Z

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 · 2023-11-25T08:13:26Z

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

ste93cry · 2023-11-26T17:55:24Z

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 · 2023-12-01T07:45:39Z

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.

dbu and others added 3 commits November 3, 2023 16:42

update with latest cs fixer

8453398

fix phpstan errors

8a0f30c

fix pending PHPStan errors

bfce8a6

Ndiritu changed the title ~~fix pending PHPStan errors~~ Fix pending PHPStan errors in https://github.com/php-http/client-common/pull/233 Nov 7, 2023

Ndiritu changed the title ~~Fix pending PHPStan errors in https://github.com/php-http/client-common/pull/233~~ Fix pending PHPStan errors in [#233](https://github.com/php-http/client-common/pull/233) Nov 7, 2023

Ndiritu changed the title ~~Fix pending PHPStan errors in [#233](https://github.com/php-http/client-common/pull/233)~~ Fix pending PHPStan errors in PR #233 Nov 7, 2023

dbu approved these changes Nov 7, 2023

View reviewed changes

MarcHagen mentioned this pull request Nov 13, 2023

PHPStan error Promise but does not specify its types: T #235

Closed

dbu mentioned this pull request Nov 22, 2023

Fix static ci #233

Merged

dbu reviewed Nov 23, 2023

View reviewed changes

This was referenced Dec 5, 2023

Accept Throwable as rejection reason php-http/promise#31

Open

use a PHPUnit bridge version that is compatible with recent PHPUnit releases php-http/stopwatch-plugin#25

Closed

dbu mentioned this pull request Jan 3, 2024

make Promise use template for exception php-http/promise#32

Closed

dbu force-pushed the fix-static-ci branch 3 times, most recently from ef61211 to ee440ee Compare January 4, 2024 19:04

dbu force-pushed the fix-static-ci branch from ee440ee to ce4e409 Compare January 4, 2024 19:08

dbu deleted the branch php-http:fix-static-ci January 4, 2024 19:09

dbu closed this Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pending PHPStan errors in PR #233 #234

Fix pending PHPStan errors in PR #233 #234

Ndiritu commented Nov 7, 2023 •

edited by dbu

Loading

dbu left a comment

dbu commented Nov 7, 2023

dbu commented Nov 8, 2023

Ndiritu commented Nov 8, 2023

Ndiritu commented Nov 15, 2023

dbu commented Nov 22, 2023

ste93cry commented Nov 23, 2023 •

edited

Loading

Ndiritu commented Nov 23, 2023

Ndiritu commented Nov 23, 2023 •

edited

Loading

dbu left a comment

dbu Nov 23, 2023

dbu Nov 23, 2023

dbu Nov 23, 2023

dbu Nov 23, 2023

dbu Nov 23, 2023

dbu Nov 23, 2023

dbu Nov 23, 2023

dbu Nov 23, 2023

dbu Nov 23, 2023

ste93cry commented Nov 23, 2023

dbu commented Nov 24, 2023

ste93cry commented Nov 25, 2023

dbu commented Nov 25, 2023

ste93cry commented Nov 26, 2023 •

edited

Loading

dbu commented Dec 1, 2023 •

edited

Loading

Fix pending PHPStan errors in PR #233 #234

Fix pending PHPStan errors in PR #233 #234

Conversation

Ndiritu commented Nov 7, 2023 • edited by dbu Loading

What's in this PR?

To Do

dbu left a comment

Choose a reason for hiding this comment

dbu commented Nov 7, 2023

dbu commented Nov 8, 2023

Ndiritu commented Nov 8, 2023

Ndiritu commented Nov 15, 2023

dbu commented Nov 22, 2023

ste93cry commented Nov 23, 2023 • edited Loading

Ndiritu commented Nov 23, 2023

Ndiritu commented Nov 23, 2023 • edited Loading

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ste93cry commented Nov 23, 2023

dbu commented Nov 24, 2023

ste93cry commented Nov 25, 2023

dbu commented Nov 25, 2023

ste93cry commented Nov 26, 2023 • edited Loading

dbu commented Dec 1, 2023 • edited Loading

Ndiritu commented Nov 7, 2023 •

edited by dbu

Loading

ste93cry commented Nov 23, 2023 •

edited

Loading

Ndiritu commented Nov 23, 2023 •

edited

Loading

ste93cry commented Nov 26, 2023 •

edited

Loading

dbu commented Dec 1, 2023 •

edited

Loading