Don't use CURLOPT_CUSTOMREQUEST for POSTs #105
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
I think the current usage of CURLOPT_CUSTOMREQUEST is wrong. As far as I can tell CURLOPT_CUSTOMREQUEST = Method::POST is not the same thing as CURLOPT_POST = true. I struggled for hours with POSTed data not being accepted by IIS. First IIS threw an HTTP 411 due to curl not sending a Content-Length header. That is solved by replacing CURLOPT_CUSTOMREQUEST with CURLOPT_POST in the case of actual POSTs. Also, IIS in my case returns an HTTP 301 redirect, which I need Unirest to follow, so I set the CURLOPT_POSTREDIR = 3 in order to let the POST follow on redirect via 301 or 302. Notice this: https://curl.haxx.se/libcurl/c/CURLOPT_CUSTOMREQUEST.html - "Many people have wrongly used this option to replace the entire request with their own, including multiple headers and POST contents. While that might work in many cases, it will cause libcurl to send invalid requests and it could possibly confuse the remote server badly. Use CURLOPT_POST and CURLOPT_POSTFIELDS to set POST data. Use CURLOPT_HTTPHEADER to replace or extend the set of headers sent by libcurl. Use CURLOPT_HTTP_VERSION to change HTTP version." In any case, I know that the CURLOPT_POSTREDIR option probably shouldn't go here and should be client configurable, but I can't find a good place for it. It would be nice to have one-shot $curlOpts passed in with the send method, or curlOpts() that otherwise live for only one curl_exec().
Remove curl_setopt(self::$handle, CURLOPT_POSTREDIR, 3); in favor of the caller being well-behaved and setting this option as needed
|
@voodoodrul looks good to me, can you just make sure all the travis tests succeed? |
|
I think Travis just timed out on some of the tests. I might need to close this PR and issue a new one to trigger another travis build. |
|
thank you |
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.
I think the current usage of CURLOPT_CUSTOMREQUEST is wrong. As far as I can tell CURLOPT_CUSTOMREQUEST = Method::POST is not the same thing as CURLOPT_POST = true.
I struggled for hours with POSTed data not being accepted by IIS. First IIS threw an HTTP 411 due to curl not sending a Content-Length header. That is solved by replacing CURLOPT_CUSTOMREQUEST with CURLOPT_POST in the case of actual POSTs. Also, IIS in my case returns an HTTP 301 redirect, which I need Unirest to follow, so I set the CURLOPT_POSTREDIR = 3 in order to let the POST follow on redirect via 301 or 302.
Notice this: https://curl.haxx.se/libcurl/c/CURLOPT_CUSTOMREQUEST.html - "Many people have wrongly used this option to replace the entire request with their own, including multiple headers and POST contents. While that might work in many cases, it will cause libcurl to send invalid requests and it could possibly confuse the remote server badly. Use CURLOPT_POST and CURLOPT_POSTFIELDS to set POST data. Use CURLOPT_HTTPHEADER to replace or extend the set of headers sent by libcurl. Use CURLOPT_HTTP_VERSION to change HTTP version."
In any case, I know that the CURLOPT_POSTREDIR option probably shouldn't go here and should be client configurable, but I can't find a good place for it. It would be nice to have one-shot $curlOpts passed in with the send method, or curlOpts() that otherwise live for only one curl_exec().