WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 3 months ago

#39494 new enhancement

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

Reported by: jason_the_adams Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: needs-testing
Focuses: rest-api Cc:

Description

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,
    );
}

Attachments (3)

39494.patch (3.5 KB) - added by jason_the_adams 14 months ago.
39494.2.diff (4.8 KB) - added by birgire 3 months ago.
39494.3.diff (8.5 KB) - added by birgire 3 months ago.

Download all attachments as: .zip

Change History (28)

#1 @jason_the_adams
18 months ago

  • Type changed from defect (bug) to enhancement

#2 follow-up: @jnylen0
18 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.8
  • Version set to 4.7

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

#3 in reply to: ↑ 2 @jason_the_adams
18 months ago

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?

#4 @jnylen0
18 months ago

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.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


14 months ago

#6 @jbpaul17
14 months ago

  • Milestone changed from 4.8 to 4.8.1

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.

#7 follow-up: @jason_the_adams
14 months ago

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

This ticket was mentioned in Slack in #core by jasontheadams. View the logs.


14 months ago

This ticket was mentioned in Slack in #core-restapi by jasontheadams. View the logs.


14 months ago

This ticket was mentioned in Slack in #core by jasontheadams. View the logs.


14 months ago

#11 @jbpaul17
12 months ago

  • Keywords needs-testing added; needs-patch removed

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


12 months ago

#13 @jbpaul17
12 months ago

  • Milestone changed from 4.8.1 to Future Release

Punting to Future Release per today's bug scrub.

#14 @Krstarica
7 months ago

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

#15 @jason_the_adams
7 months ago

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

#16 @Krstarica
7 months ago

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.

#17 @jason_the_adams
7 months ago

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
3 months ago

#18 in reply to: ↑ 7 ; follow-up: @birgire
3 months ago

  • Keywords needs-unit-tests removed

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?

#19 in reply to: ↑ 18 @jason_the_adams
3 months ago

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
3 months ago

#20 @birgire
3 months ago

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

#21 @jason_the_adams
3 months ago

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?

#22 @birgire
3 months ago

@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?

This ticket was mentioned in Slack in #core-restapi by jasontheadams. View the logs.


3 months ago

#24 follow-up: @jason_the_adams
3 months ago

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

#25 in reply to: ↑ 24 @birgire
3 months ago

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.

Note: See TracTickets for help on using tickets.