Skip to content

Disable CURLOPT_FAILONERROR to properly set body for non 2xx-responses. #14

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

Closed
wants to merge 1 commit into from

Conversation

adduc
Copy link

@adduc adduc commented Nov 12, 2016

When making a request that returns in a status code other than 2xx, Curl would
output the response to STDOUT instead of part of the response in the curl_exec
call. This would cause the Response object returned from Client::makeRequest to
contain nothing in the body. By telling curl not to fail on error, the body is
left intact in the response from curl_exec, and the body is set as expected
in the Response object returned from Client::makeRequest.

This resolves the errors being output to screen as reported in sendgrid/sendgrid-php#321.

When making a request that returns in a status code other than 2xx, Curl would
output the response to STDOUT instead of part of the response in the `curl_exec`
call. This would cause the Response object returned from Client::makeRequest to
contain nothing in the body. By telling curl not to fail on error, the body is
left intact in the response from `curl_exec`, and the body is set as expected
in the Response object returned from Client::makeRequest.
@adduc
Copy link
Author

adduc commented Nov 12, 2016

This change won't work without the fix in #13 to properly merge curl options. I added the option in the middle instead of appending to existing options to avoid merge conflicts with #13.

@thinkingserious thinkingserious added status: cla needed type: community enhancement feature request not on Twilio's roadmap labels Nov 17, 2016
@thinkingserious
Copy link
Contributor

Hello @adduc,

Thanks for the PR! We have #13 merged now.

For us to review and merge, we need a signed CLA. Can you please do that for us? Thanks!

@thinkingserious
Copy link
Contributor

Hi @adduc,

Just checking in. Do you still want this issue to be merged? Thanks!

@thinkingserious
Copy link
Contributor

Hello @adduc,

Would you like for us to make this change ourselves?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants