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

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

[dbu]

thank you for looking into this.

i start to think we should revert the widening of #30 for the 1.x version and do the change here as 2.x.

however, i am unsure about the best practices with promises: the AsyncHttpClient is supposed to only do promises for http exceptions and throw all other exceptions. if any other exception is thrown, it could imho make sense to not go through the promise and callbacks because it means some unhandled fatal error has happened that is not supposed to be handled by the stack.

unfortunately i lack experience with the promise pattern to decide which is the right approach.

[dbu]

i think this moves the BC break from our own code to the callable. if the onRejected callback has a type declaration, the fatal error would now happen while calling the callback

[ste93cry]

Technically speaking, we're not breaking anything that wasn't broken before imho: if the callback did not accept a Throwable, then it keeps being broken, otherwise we were the ones that were broken, so it didn't matter.

[dbu]

yes, correct. but thats why i wonder if we should do the widening or sharpen that a rejected promise can be about only a subset of exceptions.

[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 php-http/client-common#234 (comment) - 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 php-http/httplug#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 php-http/httplug#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.