Skip to content

[HttpClient] Let curl handle transfer encoding led to a change in behavior #57544

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.

Sign up for GitHub

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

Jump to bottom
Closed
ddegasperi opened this issue Jun 26, 2024 · 2 comments
Closed

[HttpClient] Let curl handle transfer encoding led to a change in behavior #57544

ddegasperi opened this issue Jun 26, 2024 · 2 comments
Labels
Bug HttpClient Status: Needs Review

Comments

@ddegasperi
Copy link

Symfony version(s) affected

5.4

Description

With the backport-patch #54517 to symfony 5.4 the http-client sends a empty request body to the server when using a multipart form data.

How to reproduce

With the following code snippet I receive a empty request body on the server:

$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(),
    'body' => $formData->bodyToIterable(),
]);

The client runs on a server with:
PHP Version: 7.4.33
Curl: 7.61.1

Possible Solution

A possible workaround is to add the following header Transfer-encoding: chunked to the request, but this would break once HTTP/2 protocol is used.

The patch seems to work just with a more recent version of Curl (e.g. 8.8), so an update of this dependency would be another solution

Additional Context

Since Symfony 5.4 is supported also for PHP >=7.2.5, which often runs on systems where dependencies like curl are not necessarily/easaly updatable and I wonder if it was right to backport this patch to this version as it has visibly brought about a change in behavior.
@ddegasperi ddegasperi added the Bug label Jun 26, 2024
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 27, 2024 •
edited
Loading

At the moment, I don't see any way to fix this issue in all cases.
We cannot always know ahead of time if h1.1 or h2 is going to be used so we cannot decide if we can send the header ourselves or not.
Could anyone investigate which version of curl fixed the behavior? That might be a start.

@nicolas-grekas nicolas-grekas mentioned this issue Jun 28, 2024
Merged
nicolas-grekas added a commit that referenced this issue Jun 28, 2024
@nicolas-grekas
bug #57569 [HttpClient][Mailer] Revert "Let curl handle transfer enco…
792c027 
…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
@ddegasperi
Copy link
Author

@nicolas-grekas thank you, with the new version 5.4.41 the code behaves exactly as before and has solved my problem

@xabbuh xabbuh mentioned this issue Jul 1, 2024
Merged
fabpot added a commit that referenced this issue Jul 2, 2024
@fabpot
bug #57621 [Mailer]  force HTTP 1.1 for Mailgun API requests (xabbuh)
e6c59eb 
This PR was merged into the 5.4 branch.

Discussion
----------

[Mailer]  force HTTP 1.1 for Mailgun API requests

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #57544, Fix #57569 (comment)
| License       | MIT

Commits
-------

0bf9e72 force HTTP 1.1 for Mailgun API requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpClient Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nicolas-grekas @ddegasperi @carsonbot