WP_REST_Request->parse_json_params only supports 'application/json'

[pfefferle]

The parse_json_params function only supports application/json to parse the body as JSON, that means every subtype like application/ld+json or application/activity+json will be ignored. It is also possible to add additional data like application/json; profile="https://www.w3.org/ns/activitystreams" that will be also ignored by the current implementation.

I would recomment to use a more generic check like:

preg_match( '/application\/(\w+\+)?json.*/i', $content_type )

and/or a filter to overwrite the check.

[pfefferle]

JSON Content-(Sub)Type examples:

ActivityPub ​https://www.w3.org/TR/activitypub/

• application/ld+json
• application/activity+json
• application/ld+json; profile="​https://www.w3.org/ns/activitystreams"

JF2Feed ​https://www.w3.org/TR/jf2/

• application/jf2feed+json

JSON Web Signature (JWS) ​https://tools.ietf.org/html/rfc7515

• application/jose+json

WebFinger ​https://tools.ietf.org/html/rfc7033

• application/jrd+json

Here is the complete list: ​https://www.iana.org/assignments/media-types/media-types.xhtml#application

[TimothyBlynJacobs]

Thanks for the ticket @pfefferle!

I agree this would be a good change to make, are you interested in working on a patch for this?

[ionosbot]

Sure, I will give it a try and prepare a patch.

[pfefferle]

Oops, wrong account :(

Sure, I will give it a try and prepare a patch.

[TimothyBlynJacobs]

Hey @pfefferle, just checking in. Are you still interested in working on this? Have you run into any issues?

[pfefferle]

Hey @TimothyBlynJacobs sure, I am still interested in working on this. Sorry for beeing so passive, had some busy weeks! I plan to submit a diff, the next few days.

[TimothyBlynJacobs]

No worries at all!

Add support for alternate JSON Content-Types like application/ld+json.

• List of Media Types: ​https://www.iana.org/assignments/media-types/media-types.xhtml#application
• Media-Type Naming Requirements: ​https://tools.ietf.org/html/rfc6838#section-4.2

Trac ticket: https://core.trac.wordpress.org/ticket/49404

[pfefferle]

@TimothyBlynJacobs I commited the changes via GitHub (including some tests).

Thanks for the PR @pfefferle!

Why is get_parameter_order using a different check from parse_json_params?

We'll probably also want to update wp_is_json_request.

I thought it might be more performat to only check if there is JSON in the content type, because otherwise preg_match would be called twice. But sure, I can use the preg_match in both cases.

My concern about making the parameter order check looser is that we'd be saying this request contains JSON data when really it doesn't.

Maybe introduce a protected is_json_request() method on the request object and cache the results of the regex check?

@TimothyBJacobs what do you think about the preg_match in general?

I like it!

I like the is_json_request() idea, but I think is_json_content_type() would be a better match, because it only checks the content_type and the content body still might be broken/non JSON?!?

That works for me!

@TimothyBJacobs I added a is_json_content_type method with some tests.

@pfefferle can we cache that value as long as the content-type header hasn't changed? That way we don't have to worry as much about the impact of the regex check?

@TimothyBJacobs oops, forgot about that!

We also have to worry about add_header and remove_header. It may be better to build into the cache the value of the content-type header when it evaluated it.

@TimothyBJacobs do you think it is worth it? I do think the complete "caching" and "invalidate caches" is very error-prone and it is "only" a preg_match in the end?!?

You're definitely right that is error prone. If the preg_match would only occur a couple of times in a request, I think it wouldn't be too bad. But AFAICT, this would result in a preg_match call for every single get or set of a parameter which could add up.

@TimothyBJacobs like this? we should discuss the namings!

I like that! Let's get some tests on the different ways we can modify the header.

we should discuss the namings!

Namings make sense to me! Do you have any concerns about them?

@TimothyBJacobs while writing tests, I think I found a bug. Content-Type should always contain a single type, even if you use $request->add_header();, but it works like the Accept header and adds them to a list.

`PHP
$request->add_header( 'Content-Type', 'application/json' );
$request->add_header( 'Content-Type', 'application/activity+json' );
`

Result for $request->get_header( 'Content-Type' ) is application/json,application/activity+json

Result for $request->get_content_type() is:

`
array(4) {

value?=>
string(78) "application/json,application/activity+json"
type?=>
string(11) "application"
subtype?=>
string(66) "json,application/activity+json"
parameters?=>
string(0) ""

}
`

I think we should consider that in a separate ticket. For now let's just keep operating on the first header in the list.

I only mentioned it, because it broke the test for add_header but I will rewrite them.

wp_is_json_request should use the same preg_match pattern ​https://wordpress.slack.com/archives/C02RQC26G/p1591294944063300

@TimothyBJacobs I added tests. Can you re-review?

Do you want to update wp_is_json_request in this PR as well? Would probably be good to keep them synced like you mentioned.

Sure!

Sure!

@pfefferle following up on this. If we want to include it in 5.6 I'd like to have a patch in the next few days. Ideally by Monday.

@pfefferle What about localizing the cache to wp_is_json_content_type() with a static variable? That way we can simplify the changes to WP_REST_Request, and would make caching wp_is_json_request() simple as well.

@TimothyBJacobs can you re-check?

@pfefferle This is looking good, thanks! To clarify, since wp_is_json_request can now be used by multiple consumers, it should cache any input, not just the last one. So a static $is_json[ $header ] = regex_check()

👍

[TimothyBlynJacobs]

In 49329:

REST API: Support a broader range of JSON media types.

Previously, we only supported application/json which prevented using subtypes like application/activity+json. This allows for the REST API to json_decode the body of requests using a JSON subtype Content-Type. Additionally, wp_die() now properly sends the error as JSON when a JSON subtype is specified in the Accept header.

Props pfefferle.
Fixes #49404.

Fixed in d56906f5b0fec8cce8829bb548a8c7c7e4c64b80. Thanks for the patch @pfefferle!