[HttpClient] Always set CURLOPT_CUSTOMREQUEST to the correct HTTP method in CurlHttpClient #59062
Conversation
|
The fix looks good to me. But can we also add a test to prevent regressions? |
|
I added a |
…hod in CurlHttpClient
326e259 to
bfdf5d9
Compare
|
Thank you @KurtThiemann. |
| @@ -197,13 +197,12 @@ public function request(string $method, string $url, array $options = []): Respo | |||
| $curlopts[\CURLOPT_RESOLVE] = $resolve; | |||
| } | |||
|
|
|||
| $curlopts[\CURLOPT_CUSTOMREQUEST] = $method; | |||
| if ('POST' === $method) { | |||
| // Use CURLOPT_POST to have browser-like POST-to-GET redirects for 301, 302 and 303 | |||
There was a problem hiding this comment.
@nicolas-grekas wouldn't the usage of CURLOPT_CUSTOMREQUEST defeat this comment ?
There was a problem hiding this comment.
I think it's fine because it's set after.
There was a problem hiding this comment.
I, too, thought that, but after testing it just now it seems like it might not be fine...
There was a problem hiding this comment.
I looked into this, and it seems like this would be an issue in plain cURL, but actually works correctly in Symfony because redirects are handled in CurlResponse::parseHeaderLine instead of being handled by cURL itself.
I might still make a PR to add a test for this.
This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [HttpClient] Test POST to GET redirects | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | idk | New feature? | no | Deprecations? | no | Issues | - | License | MIT In a discussion in #59062 after that pull request was already merged, it was suggested that the changes of that pull request might break POST-to-GET redirects. While it turned out that they do not because of how Symfony handles redirects, but I thought it might be a good idea to just test this. Commits ------- a96cfa4 [HttpClient] Test POST to GET redirects
Currently, when sending a
HEADrequest and using a closure as the request body usingCurlHttpClient, aPUTrequest instead of a HEAD request will be sent. This is becauseCURLOPT_NOBODYis set to make aHEADrequest, but is overwritten byCURLOPT_UPLOAD, which is necessary to make curl callCURLOPT_READFUNCTION, but also sets the HTTP method toPUT.The solution I am suggesting is to just always set
CURLOPT_CUSTOMREQUEST, so the correct HTTP method will be used, no matter what other options are added.This is mainly an issue in the PSR-18 wrapper, since PSR requests always have a request body stream, even if the body is empty. Since #58856,
Psr18Clientalways passes a closure as the request body to the underlying HTTP client instead of just reading the entire request body stream into a string, as that made it impossible to upload large files.While it would be possible to add an exception to
Psr18Clientto only add the request body if the method used commonly has a body, I think the better solution is to ensure thatCurlHttpClientalways actually sends the type of request that it is asked to send.