[HttpClient] Support file uploads by nesting resource streams in body option
#49911
Conversation
5508bb5 to
a7bcd3f
Compare
e485a34 to
45f3844
Compare
body option
6f4b407 to
c9c47dd
Compare
c9c47dd to
86bca71
Compare
| { | ||
| 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.
why is this a static variable instead of a local variable ? Do we really need a hidden global state here ?
There was a problem hiding this comment.
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.
I'm not against using a variable. But it could be a normal one, so that each call to prepareBody is independant.
There was a problem hiding this comment.
Or you should not reuse that cookie when generating the multipart boundary that is visible to the outside.
There was a problem hiding this comment.
Each call is already independent from the previous one thanks to the randomization provided by the xxh128 hashing.
I don't want to call random_bytes all the time.
| foreach ($body as [$k, $part, $h]) { | ||
| yield $part; | ||
|
|
||
| while (null !== $h && !feof($h)) { |
There was a problem hiding this comment.
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.
I'd prefer not. The current code allows the outer scope to rewind the stream if needed.
There was a problem hiding this comment.
but how do you handle closing those streams to avoid leaking them ?
There was a problem hiding this comment.
I added unset($body[$i]); and $h = null at the end of the generator. Stream resources are auto-closed by PHP when released from memory.
29b0c79 to
b6e6d17
Compare
b6e6d17 to
3555f3e
Compare
…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 #49911 Commits ------- 200027c [HttpClient] Fix global state preventing two CurlHttpClient instances from working together
3555f3e to
9745982
Compare
|
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 | MIT Issue was introduced in #49911 Commits ------- 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.