-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: Throw an InvalidRequest whenever a curl request fails #103
feat: Throw an InvalidRequest whenever a curl request fails #103
Conversation
46b9092
to
74f74c3
Compare
74f74c3
to
0204332
Compare
Hello @colinodell, I like this solution combined with #105. What do you see as the cons of choosing this solution to #104? Thanks! With Best Regards, Elmer |
Hey Elmer, I strongly prefer this option because any issues result in thrown exceptions which can be caught and dealt with. The alternate approach in #104 cannot be caught and handled by anyone using this library, it just provides a better error message instead of failing silently. The only potential con is that this is technically a backward-compatibility break as users of this library may not be expecting any exceptions to be thrown, so you'll probably want to bump the major version of this library if you're following semver. Despite this, I do think this approach is best way forward and it aligns with how other API libraries also throw expections when issues occur. |
Thanks @colinodell, I agree with you. Please fold #105 into this PR. Thanks! |
Done! I have also added some documentation via 281048f |
Hello @colinodell, |
Done! Thanks for letting me know about that, I appreciate it! |
Hello @colinodell, |
Hello @colinodell, |
Is this the same or conflict with what I already have done at #99 ? |
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.
LGTM
Fixes #90
This is one of three possible solutions to #90. #104 is similar but doesn't use catchable exceptions, and #105 is more about avoiding the issue than notifying the user.
Checklist
Short description of what this PR does:
composer/ca-bundle
(if installed) to locate the system's CA bundle or fall back to Mozilla's (Support composer/ca-bundle fallback #105)