NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[HttpClient] Handle requests with null body#45566

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:4.4fromjderusse:fix-null-body

Feb 27, 2022

Merged

[HttpClient] Handle requests with null body#45566

nicolas-grekas merged 1 commit intosymfony:4.4fromjderusse:fix-null-body

Feb 27, 2022

Conversation

Copy link

Member

jderusse commentedFeb 26, 2022•
edited
Loading

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

since#45527 passing null to thebody parameters leads to an exception (whichbreaks async-aws)

Argument#2 ($body) must be of type Closure, null given, called in /home/runner/work/aws/aws/vendor/symfony/http-client/CurlHttpClient.php on line 221

In curl client:null is not a string andself::readRequestBody expects a closure.

symfony/src/Symfony/Component/HttpClient/CurlHttpClient.php

Lines 214 to 221 in08fa74a

	if (!\is_string($body =$options['body'])) {
	if (\is_resource($body)) {
	$curlopts[\CURLOPT_INFILE] =$body;
	}else {
	$eof =false;
	$buffer ='';
	$curlopts[\CURLOPT_READFUNCTION] =staticfunction ($ch,$fd,$length)use ($body, &$buffer, &$eof) {
	returnself::readRequestBody($length,$body,$buffer,$eof);

In NativeClient,getBodyAsString will fail to returnnull because of thestring return type.

Before#45527 null was converted to"" thanks to the defaultOptions, but this is not the case anymore.

In many places, we check if the body is!== "" but rarely check if the body is null, this PR restores the original behaviors for thebody parameters and converts nulls to"".

jderusse requested a review fromnicolas-grekas

February 26, 2022 18:52

carsonbot added Status: Needs Review Bug labels

Feb 26, 2022

carsonbot added this to the4.4 milestone

Feb 26, 2022

jderusse added the HttpClient label

Feb 26, 2022

carsonbot changed the title~~Handle requests with null body~~[HttpClient] Handle requests with null body

Feb 26, 2022

jderusse mentioned this pull request

Feb 26, 2022

Do not send request with null bodyasync-aws/aws#1191

Merged

Copy link

MemberAuthor

jderusse commentedFeb 26, 2022

note: The signature of theHttpClientInterface::request method says

'body' => '' // array|string|resource|\Traversable|\Closure - description ...

null is not a valid type and the application should not have sentbody => null.

But from another hand, the same docblock says

'auth_basic' => null, // array|string - description ...

One could think thatnull is not a valid type, but could be interpreted as "I don't want to set the option"

So I believe#45527 is a BC break forbody (and maybe other options)

Copy link

Member

nicolas-grekas commentedFeb 26, 2022

So I believe#45527 is a BC break for body (and maybe other options)

You are right. I'm annoyed for other options. I'm wondering if we shouldn't revert#45527 and/or review which options need null to be overriding. Can you have a look? I will also as soon as I'm not AFK.

Copy link

Member

nicolas-grekas commentedFeb 26, 2022

Here is what I'd like to try:
On the first call to prepareRequest, store the defaults in a new property named eg $emptyDefaults.
When merging options, if the corresponding key in $emptyDefaults is null, allow null to override the defaults. Otherwise no (and keep the previous behavior). That should fix body and all affected options.
Could you please give it a try?

Copy link

MemberAuthor

jderusse commentedFeb 26, 2022

Here is what I'd like to try: On the first call to prepareRequest, store the defaults in a new property named eg $emptyDefaults. When merging options, if the corresponding key in $emptyDefaults is null, allow null to override the defaults. Otherwise no (and keep the previous behavior). That should fix body and all affected options. Could you please give it a try?

I'm not sure to understand: The first call toprepareRequest might containproxy=FOO orbody="" in the first case (proxy=FOO) we wantnull to reset proxy. In the second case (body="") we wantnull to use the default"".

I believe the right solution is to have an internal technical default (not paramterizable) that fallbacks nulls to a specific value:

body => ""
proxy => null
max_duration => INF ...
OR making our implementations resilient tonull options (addingif (isset($option['...'])) everywhere).

Copy link

Member

nicolas-grekas commentedFeb 26, 2022

The first call to prepareRequest might contain proxy=FOO

Actually not, because the first call always happens in the constructor. This call defines what you are looking for, this technical description of which values allow null.

jderusse force-pushed thefix-null-body branch fromae432f4 to6561f9eCompare

February 26, 2022 22:59

Copy link

MemberAuthor

jderusse commentedFeb 26, 2022

The first call to prepareRequest might contain proxy=FOO
Actually not, because the first call always happens in the constructor. This call defines what you are looking for, this technical description of which values allow null.

go it.. PR updated.
Actually, the method is not always called only when the defaultOption is provided (But the result is the same). And the MockClient does not have default options