Make it possible for taxonomy query to include children in REST API

[jason_the_adams]

Greetings!

Currently when a registered taxonomy is used it's assumed that include_children is false. That's a bummer as a useful bit of functionality is being cut out of the WordPress query.

<?php
if ( ! empty( $request[ $base ] ) ) {
        $query_args['tax_query'][] = array(
                'taxonomy'         => $taxonomy->name,
                'field'            => 'term_id',
                'terms'            => $request[ $base ],
                'include_children' => false,
        );
}

What about making it possible for $request[$base] to either be a string and work as-is for backwards-compatibility, or an array with term_id and include_children indexes?

As a potential solution:

<?php
if ( ! empty( $request[ $base ] ) ) {
    $terms = is_array($request[ $base ]) && isset($request[ $base ][ 'term_id' ]) ? (int) $request[ $base ][ 'term_id' ] : 0;
    $include_children = is_array($request[ $base ]) && isset($request[ $base ][ 'include_children' ]) ? (bool) $request[ $base ][ 'include_children' ] : false;

    $query_args['tax_query'][] = array(
        'taxonomy'         => $taxonomy->name,
        'field'            => 'term_id',
        'terms'            => $terms,
        'include_children' => $include_children,
    );
}

[jnylen0]

Thanks for moving this over to Trac (previously ​https://github.com/WP-API/WP-API/issues/2909).

It looks like you're referring to this code block: posts endpoint `tax_query`

I think the general approach you've proposed of accepting a string or an object is a good one - it leaves the way open for further enhancements such as querying terms by slug. Needs a few things, though:

• Patch file with ​correct spacing and ternary statements split out into if blocks for clarity
• Unit tests for old and new behavior
• Updates to arguments and schema for autodiscovery responses (something similar to content should work well here, because it also accepts a string or an object)

Also note that for the purposes of the schema/arguments, this would be type => 'object' rather than type => 'array'.

[jason_the_adams]

Replying to jnylen0:
Thanks for affirming the idea! I'll put together a patch with tests.

Also note that for the purposes of the schema/arguments, this would be type => 'object' rather than type => 'array'.

Can you please clarify what you're referring to here? What specifically is an object instead of array?

[jnylen0]

In JSON Schema, keyed arrays are objects, unlike in PHP. ​Docs; example. Just mentioning because this has bitten a couple of people in the past.

[jbpaul17]

Punting this to 4.8.1, but if it gets patched and reviewed by Friday, May 12th then we can bring it back into 4.8.

[jason_the_adams]

Here's a patch. It's note quite working yet, but I need some other eyes on this.

Presently the unit test isn't working, but I'm baffled as to why. Also, it's breaking another unit test, but I'm also unsure as to why that one is being affected (the test mostly works, but fails where it expects an error).

I'm also wondering if this should be applied to the exclude portion of the taxonomy code as well so folks can set include_children there as well.

Thank you for helping me think through this in slack, @jnylen0

[jbpaul17]

Punting to Future Release per today's bug scrub.

[Krstarica]

Default setting for include_children should be true, like ​https://wordpress.org/plugins/json-api/ has.

[jason_the_adams]

@Krstarica The reason include_children is false is for backwards compatibility. If it was true by default then existing implementations may suddenly have different results after updating.

Still really hoping to get more eyes on this and have someone help with the unit tests.

[Krstarica]

Got it. In the meantime, is there any workaround beside manually patching the file?

Having difficulties using rest_{$this->post_type}_query filter for that.

[jason_the_adams]

I believe the filter you're pointing out is the way to go. You would need to stop using the built-in query parameter and then use your own. WordPress doesn't overwrite the 'tax_query' if you don't use it.

[birgire]

Replying to jason_the_adams:

Presently the unit test isn't working, but I'm baffled as to why. Also, it's breaking another unit test, but I'm also unsure as to why that one is being affected (the test mostly works, but fails where it expects an error).

Hi @jason_the_adams, thanks for the patch. I adjusted your patch in 39494.patch​ with the draft in 39494.2.diff​ that includes two tests methods that run successfully:

• test_get_items_categories_with_updated_taxonomy_schema_and_include_children_true()
• test_get_items_categories_with_updated_taxonomy_schema_and_include_children_false()

But I'm still hesitated about updating previous schema, I wonder if new $base parameters would be needed instead, to introduce a new schema? I didn't change e.g. the schema in the WP_REST_Posts_Controller::get_collection_params().

I also wonder about alternatives to ids like terms, term_id or term_ids? And how e.g. a possible future term slug(s) filtering support would play with this and it's naming convention?

[jason_the_adams]

Replying to birgire:

But I'm still hesitated about updating previous schema, I wonder if new $base parameters would be needed instead, to introduce a new schema? I didn't change e.g. the schema in the WP_REST_Posts_Controller::get_collection_params().

I also wonder about alternatives to ids like terms, term_id or term_ids? And how e.g. a possible future term slug(s) filtering support would play with this and it's naming convention?

Thank you, @birgire, for taking the time to update the test patch! I can appreciate your concerns. If we weren't able to make it completely backwards compatible, I would have been reluctant to update the schema, but as it is I feel like it should be alright?

I'm also fine with changing ids to something else. We could do terms and then check $terms[0] to indicate whether it's an array of numbers (ids) or strings (slugs), although it may be possible for a slug to be a number? Discreet keys here are probably a good idea. If we want to go this direction, I'm happy to put together another patch for this.

Thanks again! Excited to see activity on this ticket!

[birgire]

@jason_the_adams I uploaded the 39494.3.diff​ patch where I played with a new categories_object query parameter, just to understand better how it works and to preserve the previous categories query parameter setup. Also I'm not sure it's supported to use a mixed schema (array+object) for query parameters in WP_REST_Posts_Controller::get_collection_params(). It supports term_ids, term_slugs and include_children. The term_slugs takes precedence over term_ids.

39494.3.diff​ has the tests:

• test_get_items_categories_by_term_ids_and_include_children_true().
• test_get_items_categories_by_term_ids_and_include_children_false().
• test_get_items_categories_by_term_slugs_and_include_children_true().
• test_get_items_categories_by_term_slugs_and_include_children_false().

The two tests methods in 39494.2.diff​ ran successfully in isolation, but not for e.g. the restapi group. But these four tests methods above seems to run successfully for that group.

[jason_the_adams]

Great work, @birgire. I'm still conflicted at the idea of breaking out another parameter. Seems redundant to have two parameters that have to do with querying the resource based on taxonomy.

It seems like the biggest limiter here is WP_REST_Posts_Controller::get_collection_params() and the inability to defined a mixed schema?

From the API side, I suppose this all makes sense. From an organizational standpoint, it feels janky to start building out multiple taxonomy parameters merely based on how the taxonomy is being queried. But maybe not?

[birgire]

@jason_the_adams yes I totally agree with you here ;-) I would prefer to keep same query parameters but I'm unsure regarding the mixed schema. Introducing new query parameters in 39494.3.diff​ was just me playing with it to better understand how it works ;-) What do you suggest, going forward?

[jason_the_adams]

Got it, @birgire, thanks for the clarification. :)

I chatted with @timothyblynjacobs and he pointed out that both the title (1) and content (2) accept either a string or object and disable sanitization and validation to do so. It would be great if we could use oneOf per schema convention, but that isn't supported, yet. Hopefully another ticket will come down the road to improve that.

In the next couple days I'll take your first patch and give that a go!

Footnotes:

1. ​https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L1901
2. ​https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L1926

[birgire]

Replying to jason_the_adams:

I chatted with @timothyblynjacobs and he pointed out that both the title (1) and content (2) accept either a string or object and disable sanitization and validation to do so. It would be great if we could use oneOf per schema convention, but that isn't supported, yet. Hopefully another ticket will come down the road to improve that.

I stumbled up on a great article by him here on the matter:

​https://timothybjacobs.com/2017/05/17/json-schema-and-the-wp-rest-api/

This seems to be the way forward, to support the schema keywords like oneOf and anyOf

I think we should create a new ticket for this ;-)

In the next couple days I'll take your first patch and give that a go!

I'm sure the new keywords would fix those issues ;-)

Footnotes:

Thanks for the links, interesting workarounds.

[Krstarica]

Still having hard-coded:

'include_children' => false

[stewei]

It is still hardcoded in the file ​https://developer.wordpress.org/reference/classes/wp_rest_posts_controller/ which makes it impossible to use hierarchical taxonomies via WP-REST-API.

Is there any way to work around this issue without modifing the core files?

[TimothyBlynJacobs]

We now have support for anyOf and oneOf. Does someone want to take a crack at refreshing this?

[dlh]

Sure! I could give this a shot for 5.7.

[TimothyBlynJacobs]

That'd be great! Milestoned and assigned.

[dlh]

39494.4.diff​ is a first pass at adding object schema support for both the $taxonomy and "{$taxonomy}_exclude" parameters via the new oneOf keyword.

There was some discussion above about the naming of the object property for term IDs. The patch goes with terms for parity with the WP_Tax_Query parameter and for a potentially easier addition of a sibling field property later for querying by slug or name. But I'm open to switching it to ids or similar if the plan would be to use separate oneOf schema for slug or name queries.

Given this uncertainty about how slug or name queries might be supported later, the object schema declares 'additionalProperties' => false to "reserve" the space for future core properties.

[TimothyBlynJacobs]

This is looking really great @dlh. Would you mind converting this to a PR?

I like using terms. Either way though we would probably still need a more advanced schema if we do support querying by different fields. Since we'd want to make sure terms is an array of ints when field is id. I think using separate properties implies that you could pass both ids and slugs in the same request. I don't think we could properly support that.

I think we should pull out the whole tax_query building into a separate method at this point. We might also want to put the oneOf in a variable in get_item_schema and reuse that to ensure they stay in sync. Lastly, let's make sure to give a title to each oneOf case, as well as descriptions for the cases and properties.

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

[dlh]

Thanks for the review, @TimothyBlynJacobs! A GitHub PR is linked above. I'll get started on those edits.

Do you have any suggestions for a title? Would it reference each taxonomy or be the same for each?

[TimothyBlynJacobs]

The title is used for the oneOf error messaging. So something that would make sense in that context. Perhaps Term Id List and Term Query List?

@TimothyBJacobs I've pushed some changes based on the feedback in Trac.

Couple asides:

• Like WP_Tax_Query, no attempt is being made to prevent passing include_children => true for non-hierarchical taxonomies. Do you think it's worth trying to optimize the schema so that oneOf isn't used, or has only one option, for non-hierarchical taxonomies?

• Some translated strings in the schema were using $base for the taxonomy name, which seemed potentially inaccurate (e.g., there isn't a "categories taxonomy"). I've updated those strings to use $taxonomy->name always.

Thanks @dlh01!

• Like WP_Tax_Query, no attempt is being made to prevent passing include_children => true for non-hierarchical taxonomies. Do you think it's worth trying to optimize the schema so that oneOf isn't used, or has only one option, for non-hierarchical taxonomies?

I think it'd be good to omit the include_children from the object case, but keep the oneOf.

• Some translated strings in the schema were using $base for the taxonomy name, which seemed potentially inaccurate (e.g., there isn't a "categories taxonomy"). I've updated those strings to use $taxonomy->name always.

We intentionally prefer the rest_base since that is what the field name will be.

Is that test failure a race condition? I don't think we care about order in this case so we could make that assertEqualSets. We could also probably manually set a post date?

It certainly smells like a race condition. I haven't yet seen that failure in my local environment. I'll update to assertEqualSets.

[TimothyBlynJacobs]

In 50157:

REST API: Allow for the posts endpoint include/exclude terms query to include_children.

For example the categories or categories_exclude parameters can now optionally accept an object with a terms property that accepts the list of term ids and a new include_children property which controls the Tax Query include_children field.

Props jason_the_adams, jnylen0, birgire, dlh.
Fixes #39494.

Merged in ​https://github.com/WordPress/wordpress-develop/commit/032e946633719140af9d22794a1ebfb50ed11b08.