Accept `Throwable` as rejection reason

[ste93cry]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
License	MIT

What's in this PR?

The type of the constructor argument of RejectedPromise has been widened to accept any Throwable. Also, when an Error instance is thrown (e.g. because an unexpected type is passed to the callbacks), the promise is rejected and the chain keeps going on instead of crashing the script.

Why?

When $onFulfilled or $onRejected are called, an exception or error can be thrown. In the latter case, the whole script crashes abruptly because the catch statement, that should return a RejectedPromise, accepts only an instance of Exception. Moreover, a RejectedPromise cannot be constructed from a Throwable, because the constructor argument has the same issue. Since #30 widened the argument type of the $onRejected callback, this is now even more evident due to the misleading documentation.

Before

$promise = new FulfilledPromise('Hello');
$promise = $promise->then(fn (object $value) => $value);

// PHP Fatal error: Uncaught TypeError: {closure}(): Argument #1 ($a) must be of type object, string given

After

$promise = new FulfilledPromise('Hello');
$promise = $promise->then(fn (object $value) => $value);

// $promise is a RejectedPromise

Checklist

• [x] Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

[dbu]

@GrahamCampbell @Nyholm i would be glad if you have any insights here.

i see that guzzle seems to indeed wrap any exception (though not other throwables) in its rejected promise: https://github.com/guzzle/guzzle/blob/d95d9ab74822c2ca06b31477cd6775a4a299b8e8/src/Client.php#L335

our phpdoc on the php-http async client interface claims we only wrap the http client exceptions in the promise and would directly throw others.

is that the right approach? should we adjust the async client interface to pass all exceptions through the promise? currently, the guzzle7-adapter promise wraps any non http client exception into http client exceptions: https://github.com/php-http/guzzle7-adapter/blob/03328049033fc2d9c2b2a93fb5554d519d78ce6e/src/Promise.php#L56

[ste93cry]

should we adjust the async client interface to pass all exceptions through the promise?

I would say yes. As a user, I want the promises to have a consistent behaviour, meaning that in the end I receive a FulfilledPromise or a RejectedPromise. I don't expect an error/exception to crash my script in the middle of the chain, otherwise it makes no sense to have a onRejected callback. In fact, both guzzle/promises and reactphp/promise work this way.

[dbu]

we had more discussion in https://github.com/php-http/client-common/pull/234#issuecomment-1835626700 - i think we can not suddenly change what class of exception we pass to the rejected callback. and e.g. the guzzle adapter wrapped all exceptions in httplug exceptions to work with that restriction.

[GrahamCampbell]

I don't think throwables should be wrapped. Non-exception throwables are code bugs, and typically we want a hard crash. I somewhat regret that guzzle promises made this change in a minor release. I guess I don't massively oppose this, if it's to be made in a major release.

[dbu]

looking at guzzle adapter, we adapt the guzzle PromiseInterface to our promise like this: https://github.com/php-http/guzzle7-adapter/blob/03328049033fc2d9c2b2a93fb5554d519d78ce6e/src/Promise.php#L56

i have troubles wrapping my head around what we do there but if i understand it correctly, we forward the callbacks directly to the guzzle promise, which violates the phpdoc we do on the async client, we would need to wrap the onRejected with a callback that makes sure the exception is of the right type. if its not a guzzle or psr http exception, i think indeed we should just throw it, as its not something to be expected or handled.

[dbu]

i spent a lot of time trying to get this to work in https://github.com/php-http/httplug/pull/173 but was not able to work it out. i came to the conclusion that it probably just can't work because Promise::then returns another Promise which can be of the same type or different, i think.

we can leave this open if you want to give it another shot, but I will only merge generic annotations if we have a pull request like https://github.com/php-http/httplug/pull/173 that uses this branch here and shows it can actually be used correctly.

[slepic]

Hi,

I was just randomly looking through the code and noticed the clash of exception vs. throwable in the implementation. I thought I could submit a fix while I noticed I'm not the first to have the exact same idea.

So I'll just leave my viewpoint here.

I must 100% side with @ste93cry in this case.

Calling then() should absolutely never throw (unless called with noncallback arguments).

All other exceptions (including type errors arising from callbacks with incompatible arguments) should be thrown in wait().

Promise is an abstract concept (not even restricted to http) with 3 states and it must behave the same in all of them.

It seems to me you can't but think of promise as fulfilled or rejected. Consider how a pending promise would behave when then() is called on it. It can't invoke the callbacks yet until fulfilled or rejected. And so a fulfilled/rejected promises' then() method can throw unexpected errors. While a (hypothetical) pending promise would pospone throwing these errors to the wait() method.

This is inconsistent and a clear sign that the current implementation is wrong.

This is further strengthened by the fact that other php promise implememtations also behave this way, which was already mentioned. Also javascript promises behave the same.

In conclusion, I hope this PR will pass, even if it means 2.0.