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

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.

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.

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.

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 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.