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] Support file uploads by nesting resource streams inbody option#49911
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
e485a34 to45f3844Comparebody option6f4b407 toc9c47ddCompare| { | ||
| if (\is_array($body)) { | ||
| array_walk_recursive($body, $caster = static function (&$v) use (&$caster) { | ||
| if (\is_object($v)) { | ||
| static $cookie; |
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.
why is this a static variable instead of a local variable ? Do we really need a hidden global state here ?
nicolas-grekasApr 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
We need this to generate a random boundary without consuming too much entropy from the system.
We don't care about the hidden state because this is just a random string that is updated after use.
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.
I'm not against using a variable. But it could be a normal one, so that each call toprepareBody is independant.
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.
Or you should not reuse that cookie when generating the multipart boundary that is visible to the outside.
nicolas-grekasApr 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Each call is already independent from the previous one thanks to the randomization provided by thexxh128 hashing.
I don't want to callrandom_bytes all the time.
Uh oh!
There was an error while loading.Please reload this page.
| foreach ($body as [$k, $part, $h]) { | ||
| yield $part; | ||
| while (null !== $h && !feof($h)) { |
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.
shouldn't we close the resource once we are done reading it ? Due to the async nature of the component, it would be very hard to keep the ownership of the resource in the caller code while avoiding to close it too early (it must not be closed until http-client is done with it).
Guzzle has a feature allowing to create a body from a stream, and it takes ownership of the resource for such reason.
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.
I'd prefer not. The current code allows the outer scope to rewind the stream if needed.
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.
but how do you handle closing those streams to avoid leaking them ?
nicolas-grekasApr 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I addedunset($body[$i]); and$h = null at the end of the generator. Stream resources are auto-closed by PHP when released from memory.
29b0c79 tob6e6d17Compare…t instances from working together (nicolas-grekas)This PR was merged into the 5.4 branch.Discussion----------[HttpClient] Fix global state preventing two CurlHttpClient instances from working together| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | Fix -| License | MIT| Doc PR | -Spotted while working on#49911Commits-------200027c [HttpClient] Fix global state preventing two CurlHttpClient instances from working together
Thank you@nicolas-grekas. |
…body is empty (nicolas-grekas)This PR was merged into the 6.4 branch.Discussion----------[HttpClient] Don't send any default content-type when the body is empty| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | -| License | MITIssue was introduced in#49911Commits-------4a475e0 [HttpClient] Don't send any default content-type when the body is empty
This PR makes it easy to upload files using
multipart/form-data.Nesting streams in option "body" is all one needs to do:
By default, the code will populate the
filenameusing the base-name of the opened file.It's possible to override this by calling
stream_context_set_option($h, 'http', 'filename', 'the-name.txt').When forwarding an HTTP request coming either from the native
httpstream wrapper or from Symfony'sStreamableInterface, the code will forward the content-type. It's also possible to set the content-type usingstream_context_set_option($h, 'http', 'content_type', 'my/content-type').When possible, the code will generate a
Content-Lengthheader. This enables the destination server to reject the request early if it's too big and it allows tracking the progress of uploads on the client-side.