Skip to content
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

Throw InvalidRequest exception on invalid CURL request #99

Merged

Conversation

alextech
Copy link
Contributor

@alextech alextech commented Jun 8, 2018

Fixes

#90

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Currently, if there are any issues with requests, the library exists silently with response code 0. This creates confusion about whether the problem was detected at SendGrid server or locally, what should be done next. This PR introduces an exception thrown with message from CURL about why actually request fails.
  • Not doing a specific exception as outlined in Document and throw an error to help people with CURLOPT_SSL_VERIFYPEER issues #90 because
    1. there are many reasons why request (SSL or not) could fail, and that was just a single example. No point replicating every possible CURL error into an exception.
    2. Difficult to unit test full working connection to replicate network conditions.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Jun 8, 2018
@SendGridDX
Copy link

SendGridDX commented Jun 8, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 8, 2018

Codecov Report

Merging #99 into master will increase coverage by 3.29%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #99      +/-   ##
=======================================
+ Coverage    84.7%   88%   +3.29%     
=======================================
  Files           3     4       +1     
  Lines         170   175       +5     
=======================================
+ Hits          144   154      +10     
+ Misses         26    21       -5
Impacted Files Coverage Δ
lib/Exception/InvalidRequest.php 100% <100%> (ø)
lib/Client.php 85.31% <100%> (+3.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9a04d9...9d6ea4d. Read the comment docs.

@Rezouce
Copy link

Rezouce commented Jul 3, 2018

Hey,
Do we have any idea on when this PR will be merged?

@thinkingserious
Copy link
Contributor

Hello @Rezouce,

I do not, but for this issue to gain priority in our backlog, we need additional +1's or comments.

My apologies for the delay and thanks for your patience.

With Best Regards,

Elmer

Copy link

@DvdGiessen DvdGiessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reviewed these changes and they're basically what I envisioned when I wrote sendgrid/sendgrid-php#328 (comment). Looks good to me, lets get this moving!

@reFX-Mike
Copy link

Why is this not merged already? Can someone for Sendgrid please explain what the holdup is? This is an important issue that needs to be fixed.

@thinkingserious thinkingserious merged commit db6d34e into sendgrid:master May 7, 2019
@thinkingserious
Copy link
Contributor

Hello @alextech,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@reFX-Mike
Copy link

Now somebody just needs to push this to packagist, so we can actually benefit from this fix.

@thinkingserious thinkingserious added status: ready for deploy code ready to be released in next deploy type: community enhancement feature request not on Twilio's roadmap and removed status: code review request requesting a community code review or review from Twilio labels May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: ready for deploy code ready to be released in next deploy type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants