Enforce throwables/exceptions as rejection reasons

[jsor]

Because rejections are the promise counter parts of throwing an exception, throwables/exceptions should be enforced as rejection reasons.

Also, ReactPHP has always used exceptions as rejection reasons throughout it's components.

Closes #46

[kelunik]

How about MultiReasonException?

[clue]

No objections to either class name, but the use of this class appears a bit unclear. Does it make sense to add documentation for this class and/or mark this class a @internal?

[jsor]

Added documentation in 07b7afd. I think it makes sense to keep it public API as it might be useful for custom collection functions.

[clue]

I like the idea of only accepting Exception here, but can you elaborate on the rationale to also support Throwable? Thanks!

[jsor]

@clue I thought about that myself, but if only Exceptions are accepted, it's not possible to forward rejections easily.

ini_set('assert.exception', 1);

$promise = new \React\Promise\Promise(function() {
    assert(1 === 0);
});

$nextPromise = $promise->otherwise(function($e) {
    handle($e);

    // $e is AssertionError and can't be passed to reject()
    return \React\Promise\reject($e);
});

A solution would be to just rethrow, but that needs to be documented (and the documentation read by the user).

$nextPromise = $promise->otherwise(function($e) {
    handle($e);

    throw $e;
});

[kelunik]

@clue Why would one want to not support Throwable?

[WyriHaximus]

Explicitly not supporting Throwable's would make this PR useless in PHP 7 environments when TypeErrors bubble up, or anything else from PHP implementing Throwable, for example Error

[stefanotorresi]

isn't the point of this PR to be "not longer possible to reject a promise without a reason
or with another promise." ?
FWIW I think it makes perfectly sense to support Throwables. What's the counter argument?

[clue]

I'm 👍 on getting this in and didn't mean to question whether supporting Throwable is useful here but rather that this is not exactly clear from a consumer perspective (checking just the README). Does it make sense to describe this feature and its implications in the README? 👍

[jsor]

@clue In fact, \Exception is only mentioned for PHP 5 support. Where do you want to see more documentation and about what exactly?

[jsor]

Ping @clue

[clue]

Good catch! I think this feature and corresponding BC break makes perfect sense 👍

I have a couple of questions regarding the new custom exception classes, otherwise LGTM 👍

[clue]

No objections to either class name, but the use of this class appears a bit unclear. Does it make sense to add documentation for this class and/or mark this class a @internal?

[kelunik]

@jsor Would you be up for standardizing the CompositeException with us @ FIG? We have a MultiReasonException that basically does the same. Non-async libraries might also have a use if they invoke several callbacks and only throw an exception for all failed ones afterwards. Standardization could help with building support for this into exception reporting libraries, e.g. symfony/symfony#25883.

[jsor]

I've added [WIP] to the PR title. We still need to figure out how we can keep interoperabilty with other promise implementations which allow rejecting with non-exception reasons.

new Promise(function($resolve, $reject) use ($foreignPromise) {
    $foreignPromise->then($resolve, $reject);
});

This would now reject Promise with a \InvalidArgumentException in case $foreignPromise is rejected with a non-exception.

One solution would be to wrap the reason in a ReasonException like i did here:

https://github.com/jsor-labs/pact/blob/8ec4ea971c9cc08676081d0b5998def26dc4a165/src/Promise.php#L629-L637

[clue]

We still need to figure out how we can keep interoperabilty with other promise implementations which allow rejecting with non-exception reasons.

Do we have a relevant use case where this happens?

[jsor]

@clue Promise\resolve() for example

promise/src/functions.php

Lines 11 to 21 in c1aad8e

	if (method_exists($promiseOrValue, 'then')) {
	$canceller = null;
	if (method_exists($promiseOrValue, 'cancel')) {
	$canceller = [$promiseOrValue, 'cancel'];
	}
	return new Promise(function ($resolve, $reject) use ($promiseOrValue) {
	$promiseOrValue->then($resolve, $reject);
	}, $canceller);
	}

[clue]

@jsor It's my understanding that if a foreign promise rejects with something that is not an Exception/Throwable, the reject() method will automatically turn this into an InvalidArgumentException with an appropriate message, no? This sounds like a good approach to me!

As far as I can tell, the ecosystem of foreign promises also seem to rely on Exceptions for the most part. Consumers that actually need foreign promises with custom rejection reasons can still use an explicit foreign rejection handler and turn this into an appropriate Exception type themselves.

Are you seeing anything that is missing here and/or is commonly used in other projects?

[WyriHaximus]

LGTM !

[jsor]

ping @clue

[clue]

Changes LGTM! 🎉

[clue]

One thing: Once this is in, I would like to look into enforcing a Throwable type hint here instead of manually throwing an InvalidArgumentException. This will likely affect PHP version compatibility, so I think it's best to discuss this in a follow-up ticket 👍

[WyriHaximus]

One thing: Once this is in, I would like to look into enforcing a Throwable type hint here instead of manually throwing an InvalidArgumentException. This will likely affect PHP version compatibility, so I think it's best to discuss this in a follow-up ticket

Well either of us can do that, but I'd like to get an PR out A.S.A.P. adding exactly that and raising minimum PHP version to 7.0

[clue]

@WyriHaximus Go for it!

[WyriHaximus]

@clue 🎉 !