Support for sending a json request with empty body

[djcvijic]

Otherwise a malformed request is sent (body does not match content-length header)

[anlutro]

Hey! This makes sense, and is clearly a serious bug, but I think the fix is a bit trickier than just this.

First, I think changing the default from null to array() would be a breaking change. The current behavior might be weird but changing it now could make it even weirder. I'd prefer just to fix the content-length issue.

Second, if the default is array(), or if you pass in array() as the data, should that be [] or {} in JSON? Probably the user wants to be able to control this... Is there a way we can allow that?

[djcvijic]

@anlutro Yes, I should have given more info on this. Allow me to go into more detail, and then I'd ask you to please help find a non-breaking solution.

The first and main issue is in the commit message - application/json requests with a post-like method MUST have a body. The server will expect it, and will not respond if a body is not found. This is usually no problem because json_encode ALWAYS returns a non-empty string for any valid input, for example '[]' for empty array. One solution was to simply make Request::hasData() always return true for json-encoded post-like requests (in fact, this worked before 1.4.4 and was broken in e65f127).

But I thought a bit more, and I think it's reasonable to assume the same rule would apply for any theoretical encoding where the $data was empty but encodeData() was non-empty, so I changed the hasData() method and am quite happy with that choice.

As for $data = null or $data = array(), you can see why I did that if you change it back and run phpunit. The problem arises on line 400 of Request.php, because http_build_query ONLY allows objects and arrays, and it is now receiving null when $data is null. Another possible fix is to instead change line 400 to something like

return (!is_null($this->data) ? http_build_query($this->data) : '');

(which would still leave the question, what if somebody tries $curl->post('https://some.url', 'hello') because the scalar value 'hello' would produce the same error - what should this library do then?)

Or alternatively, change hasData() again, to

public function hasData()
{
	return (static::$methods[$this->method] && (bool) $this->encodeData());
}

So that encodeData() is never called for non-post-like requests. However, this would not stop users from simply calling $curl->post('https://some.url', null), and triggering the same bug as $data = null...

And I can go around and around with this, but I'd rather have your input :)

As for sending empty array or object in json, do not worry at all, it is a typical use case: json_encode(array()) produces '[]', json_encode(new \stdClass()) produces '{}'.

[djcvijic]

@anlutro Sorry for such a long message, here's a TL;DR:

1. Request::hasData() should check encoded data, I feel good about that change
2. If we revert cURL::prepareRequest() and allow passing $data as null, we need a solution for line 400 in Request.php, for example

return (!is_null($this->data) ? http_build_query($this->data) : '');

3. Don't worry about sending empty array vs empty object as JSON, there's a simple built-in solution for that already

[anlutro]

Hey, don't worry about being TLDR, I've just been occupied with other things so haven't had the chance to properly process this yet. Let me get back to you within a day or two, okay?

[anlutro]

Okay I think I get this. One thing you mention confuses me though - content-length being wrong. We don't set this anywhere in this library, so where is that coming from?

[djcvijic]

A few headers are set implicitly by the curl library, and I dug a bit further into this and found out that the most problematic combination is setting CURLOPT_POST=1 without setting CURLOPT_POSTFIELDS, here's what happens:

CURLOPT_POST=1 does a few things at once - sets the request method to POST if no other if specified, sets content-type to 'application/x-www-form-urlencoded' if no other is specified, then sets content-length to -1 (some default value I guess) if no other is specified.

When you set CURLOPT_POSTFIELDS also, the post body is generated and content-length is calculated and set to the correct value.

A nasty side effect of NOT setting CURLOPT_POSTFIELDS is that content-length stays at -1, which is fully invalid, probably gets cast to unsigned integer and interpreted as MAX_INT, and the server won't even return a response. Another side-effect which I noticed but not sure how important it is, is that I guess content-length=-1 causes an "Expect: 100-continue" header to be added to the already malformed request.

As you see, CURLOPT_POST should never be used without CURLOPT_POSTFIELDS. In fact, in this library I think it should not be used at all, since setting CURLOPT_POSTFIELDS in combination with always setting CURLOPT_CUSTOMREQUEST does all the necessary work for any request method, without any of the risk.

I've updated the PR, reverting to $data=null, but also adding some more tests to cover more ground, and used what I've learned about the curl lib to improve cURL::prepareRequest().

[anlutro]

Yikes, what a mess.

[anlutro]

What do you think about moving this null check to the start of this method and return an empty string for every encoding if $data === null?

[djcvijic]

For raw, it would make sense. However for json, I expect null to encode as the string "null".

[anlutro]

Good point.

[anlutro]

Maybe check the content-length header in all these new tests, to check for the bug you originally wanted to fix?

[djcvijic]

Just the fact that the request is executed successfully without any errors or exceptions means that content-length was correct, any more checks would be redundant.
In fact, you can see that a few of these tests fail if you revert the changes in the source code and try to run them, to assert that they do cover this case.

[anlutro]

Ah, okay, good to know. I don't have php installed locally anymore so I don't have the option to check myself :o

[anlutro]

Thanks a bunch for your work!

[djcvijic]

You're welcome, and I appreciate the input! I'm happier with how it turned out now than the first commit.

Might I ask whether you're planning to bump the library version some time soon, or should I specify dev-master in my projects?

[anlutro]

Tagged and released 1.4.5 with your fix right after the merge :D