-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Widen the Exception type to Throwable #2495
Conversation
Not necessarily. It might not make sense to catch throwables, actually. Throwables that are not exceptions are errors in the code, and probably should totally crash everything, on purpose. |
Yes for some of the cases of the current PR I have second thoughts also.. can you be more specific.. because I don't think that this applies everywhere.. For instance on my current PR I believe that MockHandler should accept |
@GrahamCampbell 's comment refers to catching |
Yes.. I had second thoughts too. So shall I revert every |
No, I don't think you should. For example: it's probably a good thing, that promises can now catch errors too, and return them as rejection. But I'm not sure catching errors and returning them as |
OK.. then can we have specific comments per case? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much agree with @GrahamCampbell. Catching exceptions and catching throwables are NOT the same thing.
Looking at the PR, all the changes makes sense. Like MessageFormatter or how we catch exceptions int $options['on_headers']($response)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr I would revert catching Throwable
s for now, but it should be accepted as an argument everywhere. We can revisit this decision later.
I should have these changes included on this: #2493 but I thought about later.
Please review thoroughly the code.. there might be more places that need a change.
Thank you