Support \Throwable when setting previous exception from server callback

[jsor]

Followup for #152

[clue]

Thanks for spotting! PHP's docs actually allow a Throwable here (checked the German docs before which seem to be outdated currently, duh).

I guess we should also mention this in our README?

[andig]

Won't Throwable kill php prior to 7?

[jsor]

@clue What do you mean exactly? This just sets the $previous argument for the emitted \RuntimeException.
@andig Not sure what you mean. Can you elaborate?

[maciejmrozinski]

@jsor I think, what @andig had in mind, was that Throwable is not defined in PHP < 7.0 and how such situation will be handled - fatal or just false statement.

@andig For PHP < 7.0 it will just evaluate to false.

[andig]

@maciejmrozinski Indeed, verified using l3v4.

[jsor]

@maciejmrozinski Thanks for the clarification! Yes, with PHP5 \Throwable just isn't declared and the instanceof check will evaluate to false.

[clue]

The changes LGTM, should this be mentioned in the README (see error event)?

[jsor]

The question is rather if we should completely switch over to use \Throwable as type hints in the documentations across our components?

[NanoSector]

Hit this wall too. Setting this up would mean stuff like \Error would get passed to the previous exception parameter too, and therefore generic PHP errors can be found.

[clue]

I agree that we should get this in, but we should make sure this is mentioned in the README (which only mentions Exception currently). Ultimately we may want to switch to Throwable only, but that would involve dropping PHP < 7.0, which I'd rather discuss separately (reactphp/reactphp#374) 👍

[jsor]

I agree that we should get this in, but we should make sure this is mentioned in the README (which only mentions Exception currently).

@clue Yes, and that is correct because the server will always emit a \RuntimeException. This patch only handles the $previous argument for this exception.

[clue]

[…] the server will always emit a \RuntimeException. This patch only handles the $previous argument for this exception.

I think this should be mentioned in the README 👍

In particular, it currently says (emphasis mine):

The server will also emit an error event if you return an invalid type in the callback function or have a unhandled Exception. If your callback function throws an exception, the Server will emit a RuntimeException and add the thrown exception as previous:

[jsor]

To properly support Throwables, react/promise: ^2.3 is required as that version introduced support for it.

Should we bump the version in composer.json or add a note to the readme?

[WyriHaximus]

@jsor bumping sounds like a good idea to me 👍

[WyriHaximus]

🎉