-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpClient] Let curl handle transfer encoding #54517
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
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
Thanks for investigating and for the reproducer. Could this be related to some curl bug? Which exact version of curl and php do you have ? ( Mine:
|
Hey Nicolas, that's interesting!
I checked the
While I'm not sure this is the bug I'm experiencing, there is a relevant quote:
That is the behavior this pull request would address. I just checked the servers where I first experienced #54491 and they are also on curl |
Thank you @michaelhue. |
Since this patch, our HTTP client does not add transfer-encoding: chunked header when specifying the body as a callable We have to force the header to make it work, is this a problem between curl version and this patch ? FYI : php version : 8.3.6 |
I've to apply also the "Transfer-encoding: chunked" header to make my request (see code snipped) work again after this patch: $formData = new FormDataPart([
'degree' => '90',
'file' => new DataPart(
$fileResource, // a resource not a string
'image.jpg',
'image/jpeg'
)
]);
$response = $client->request('POST', '/api/image/rotate/', [
'headers' => [
...$formData->getPreparedHeaders()->toArray(),
'Transfer-encoding: chunked' // with this header the patch works, without it does not send the data
],
'body' => $formData->bodyToIterable(),
]); PHP Version: 7.4.33 Do we know from which Curl version we should apply the following behavior?
Since Symfony 5.4 is supported also for PHP >=7.2.5, which often runs on systems where these dependencies are not necessarily updatable, I wonder if it was right to backport this patch to this version? |
Closed issues cannot be tracked so please open one to describe the bug |
@ddegasperi this solution might break once we fix the bug |
FYI updating to curl 8.8 fixed this issue for me |
FYI I think we should revert this, see #57569 |
…ding", use HTTP/1.1 for Mailgun (nicolas-grekas) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [HttpClient][Mailer] Revert "Let curl handle transfer encoding", use HTTP/1.1 for Mailgun | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #57544 | License | MIT This PR reverts #54517 and fixes #54491 by forcing HTTP/1.1 when talking to Mailgun The issue that #54517 tries to fix shouldn't happen since curl 7.51, which dates back from 2016 (curl/curl@d4c5a91). If you experience it, try upgrading. The previous code worked fine for years but the new one generated reports a few weeks after its release so I'd feel more confident reverting. Commits ------- efc93cd [HttpClient][Mailer] Revert "Let curl handle transfer encoding", use HTTP/1.1 for Mailgun
Removes the
Transfer-Encoding: chunked
header that is added for chunked request bodies. This will allow curl to handle the transfer encoding and set appropriate headers based on the HTTP protocol version it negotiated with the server, since HTTP/2 does not support chunked transfer encoding.