Don't throw exception on empty JSON body

[michaelmoussa]

2.0.3 introduced an issue where a request along the lines of PATCH /foo with
Content-Type: application/json but no actual request body (since there may not
actually be any information to provide) would result in an exception. The
previous behavior was that the request would be allowed and the Parsed Body
would be NULL.

@weierophinney this pertains to what we discussed briefly via e-mail a while back.

Other possible alternatives would be to either require the user to use text/plain (or something else) as the content type when not needing to provide a request body in order for the JsonStrategy to not be executed (which would be a bit of a hassle if 100% of all other requests were application/json) OR to require the user to provide a valid empty JSON structure ({}, '""', etc...), which would also be a bit of a bother, since the intent seems clear ("do this thing which doesn't require any further information to be provided as input from me").

I opted for NULL as the resulting parsed body rather than an empty array or empty object type since NULL was the end result prior to 2.0.3, and I figure that's one fewer vector for BC breakage.

[michaelmoussa]

@xtreamwayz @weierophinney can I get some feedback on this before merging please?

[geerteltink]

Path is used for partial updating a partial resource. Doesn't that mean that there should always be data? If there is no data, it can't update a resource and should be considered an invalid request.

[michaelmoussa]

Not necessarily. The request could be something like: PATCH /report/123/submit i.e. submit report 123 and make some pre-defined change to its state.

Sort of like: PATCH /report/123 {"state": "submitted"}, except this one looks like a simple CRUD operation and doesn't necessarily communicate that the request is going to cause something to be submitted (perhaps via e-mail or something) versus just changing a property on an entity.

That's how this issue arose, actually. The request does modify the entity, so GET isn't appropriate, but the vast majority of the API calls for application/json, so it'd be awkward to decide between text/plain or application/json depending on the request, because in either case there really isn't any additional data to provide.

[geerteltink]

I wouldn't require text/plain this might lead to confusion if you want to submit changes to an endpoint and suddenly you need to from application/json to text/plain. Also forcing a empty json body wouldn't make sense in this case.

The question is would this be considered a valid endpoint? To me it wouldn't. I would write it as you did: PATCH /report/123 {"state": "submitted"}. I've looked briefly into the specs and couldn't find if the body may be empty. If this should be considered a valid endpoint action than it should be fixed. If it's not a valid endpoint, they can extend the class and change it in such way to make it work for them.

[michaelmoussa]

The question is would this be considered a valid endpoint?

RFC 7159 states:

A JSON value MUST be an object, array, number, or string, or one of
the following three literal names:

 false null true

The wording is a bit unfortunate as it implies that NULL and booleans are OK, but it does seem to insist that null be represented as a string value that says "null", rather than no value at all.

Which makes something like this perfectly valid:

$ curl -H "Content-type: application/json" -X PATCH http://example.com/ -d 'null'

(which looks horrendous imo with that -d 'null'!)

Prior to 2.0.3, an empty request body with application/json as the content type was accepted and it was assumed that meant null. The fact that it throws an exception now is technically a BC break for anybody who relied on that behavior, which I'm still not convinced is strictly wrong. Being so strict about this particular edge case when the intent is clear (i.e. "there is no data. it's null") seems harmful to me, as there doesn't seem to be much to gain from that strictness.

Would really like your input on this as well @weierophinney :)

[weierophinney]

The only change I'd make is s/!empty/! empty/; that'll eventually be caught with the updated coding standard, however.

In terms of your arguments, I tend to agree with your assertion that an empty body should result in null for the parsed body, and not an exception. Body content for any HTTP method is always considered optional, regardless of the content type provided during the request.

I might suggest one optimization: perhaps skip parsing if the $rawBody is empty to begin with?

[michaelmoussa]

@weierophinney I'm inclined to leave the parsing as-is for two reasons:

1. json_last_error() is global. It's possible for a json_* error to occur somewhere before this helper is executed. If that happens, and we skip parsing for empty $rawBody, the exception will be thrown when it shouldn't be.
2. 10,000 iterations of json_decode('', true) adds less than 1ms of execution time on PHP 7, so it's really a micro-optimization. That alone isn't reason enough not to do it, but if we add an extra check to avoid the unlikely-but-possible issue in point 1, we'll shrink the micro-optimization even further and lose a bit of clarity. Not worth it imo.

I'm fine with it if you don't think the concerns are warranted, though, but in the meantime I'll merge this since you already approved it.