Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpClient] Stream request body in HttplugClient and Psr18Client#58856
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedNov 13, 2024
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution 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 |
(please rebase, not merge) |
Sorry, should be fixed Any idea what's up with the tests? I'd say I'm reasonably confident I'm not responsible for all the failures here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Works for me as an improvement, thus for 7.2
Uh oh!
There was an error while loading.Please reload this page.
Note that this means the content-length won't be sent anymore and the chunked encoding will be used instead. |
The Content-Length header should now be added if |
Uh oh!
There was an error while loading.Please reload this page.
Not sure if we'd want to do something about the duplicate code in Psr18Client and HttplugClient |
Thank you@KurtThiemann. |
bbdf0e0 intosymfony:7.2Uh oh!
There was an error while loading.Please reload this page.
I think that the documentation should say |
…ct HTTP method in CurlHttpClient (KurtThiemann)This PR was submitted for the 7.2 branch but it was squashed and merged into the 6.4 branch instead.Discussion----------[HttpClient] Always set CURLOPT_CUSTOMREQUEST to the correct HTTP method in CurlHttpClient| Q | A| ------------- | ---| Branch? | 7.2| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#59043| License | MITCurrently, when sending a `HEAD` request and using a closure as the request body using `CurlHttpClient`, a `PUT` request instead of a HEAD request will be sent. This is because `CURLOPT_NOBODY` is set to make a `HEAD` request, but is overwritten by `CURLOPT_UPLOAD`, which is necessary to make curl call `CURLOPT_READFUNCTION`, but also sets the HTTP method to `PUT`.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, `Psr18Client` always 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 `Psr18Client` to only add the request body if the method used commonly has a body, I think the better solution is to ensure that `CurlHttpClient` always actually sends the type of request that it is asked to send.Commits-------bfdf5d9 [HttpClient] Always set CURLOPT_CUSTOMREQUEST to the correct HTTP method in CurlHttpClient
Currently,
Psr18ClientandHttplugClientwill simply callgetContents()on the stream, loading the entire stream content into a string before passing it to the underlyingHttpClient. When sending a request with a large body, this can lead to the PHP process running out of memory.This pull request changes both clients to instead pass a Closure that reads from the request body stream to the underlying
HttpClient.The HttpClient documentation states that "each time, the closure should return a string smaller than the amount requested as argument", which would mean that if the closure was called requesting a size of 10 bytes, you are only allowed to return at most 9 bytes. I assume that is an inaccuracy in the docs, but please correct me if I'm wrong here.
I'm also not fully sure whether this would be considered a new feature or a bug fix.