Skip to content
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

[HttpClient] Support file uploads by nesting resource streams in body option #49911

Merged
merged 1 commit into from Apr 20, 2023

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR makes it easy to upload files using multipart/form-data.

Nesting streams in option "body" is all one needs to do:

$h = fopen('/path/to/the/file' 'r');
$client->request('POST', 'https://...', ['body' => ['the_file' => $h]]);

By default, the code will populate the filename using 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 http stream wrapper or from Symfony's StreamableInterface, the code will forward the content-type. It's also possible to set the content-type using stream_context_set_option($h, 'http', 'content_type', 'my/content-type').

When possible, the code will generate a Content-Length header. 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.

@carsonbot carsonbot added this to the 6.3 milestone Apr 3, 2023
@nicolas-grekas nicolas-grekas force-pushed the hc-multipart branch 5 times, most recently from e485a34 to 45f3844 Compare April 3, 2023 14:37
@OskarStark OskarStark changed the title [HttpClient] Support file uploads by nesting resource streams in option "body" [HttpClient] Support file uploads by nesting resource streams in body option Apr 6, 2023
@nicolas-grekas nicolas-grekas force-pushed the hc-multipart branch 2 times, most recently from 6f4b407 to c9c47dd Compare April 7, 2023 14:42
{
if (\is_array($body)) {
array_walk_recursive($body, $caster = static function (&$v) use (&$caster) {
if (\is_object($v)) {
static $cookie;
Copy link
Member

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 ?

Copy link
Member Author

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.

Copy link
Member

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 to prepareBody is independant.

Copy link
Member

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.

Copy link
Member Author

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 the xxh128 hashing.
I don't want to call random_bytes all the time.

src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated Show resolved Hide resolved
foreach ($body as [$k, $part, $h]) {
yield $part;

while (null !== $h && !feof($h)) {
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nicolas-grekas nicolas-grekas force-pushed the hc-multipart branch 4 times, most recently from 29b0c79 to b6e6d17 Compare April 20, 2023 12:55
nicolas-grekas added a commit that referenced this pull request Apr 20, 2023
…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
@fabpot
Copy link
Member

fabpot commented Apr 20, 2023

Thank you @nicolas-grekas.

@fabpot fabpot merged commit e957514 into symfony:6.3 Apr 20, 2023
8 of 9 checks passed
@nicolas-grekas nicolas-grekas deleted the hc-multipart branch April 20, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants