Catch Exception instead of Throwable #147
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks like even this broke the test so I'll have to spend a bit more time on it... Still though this is not the first time where I saw this lib failing with simple connection closed, hiding the real issue completely. So I do believe something should be changed. |
|
Good catch, thanks 👍 |
|
Well guess what... those errors that popped up now are genuine PHP errors that were always there just completely hidden. The first one (and I'm really unsure how to fix it) is here. |
|
@prolic Would you mind taking a look at this? From the code I really have no idea what your intention was. |
|
Yes, will do. |
Pull Request Test Coverage Report for Build 795
💛 - Coveralls |
|
Don't merge yet, I wanna check this again this weekend or so. (Quite busy now.) |
|
Perfect. Thanks for your help
…On Mon, Nov 2, 2020, 18:59 Jáchym Toušek ***@***.***> wrote:
Don't merge yet, I wanna check this again this weekend or so. (Quite busy
now.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#147 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADAJPD7QBYI5BWXSJ5JO5TSN4TSTANCNFSM4S43SH7Q>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When trying this lib together with amphp v3 I ran into a problem where a type error of passing a wrong argument somewhere was completely hidden and the test failed with simple "Connection closed" exception with no further indication of what went wrong.
After a while I found out that the type error was caught here and completely hidden by this library's error handling. In my opinion it's better to not catch
ErrorException- those tend to be real bugs that you want to see right away.Personally I'd be much more careful with catching
Exceptionas well but that would require more knowledge.