WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

Last modified 8 months ago

#39494 closed enhancement (fixed)

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

Reported by: jason_the_adams Owned by: dlh
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: needs-testing has-patch has-unit-tests
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 (5)

39494.patch (3.5 KB) - added by jason_the_adams 4 years ago.
39494.2.diff (4.8 KB) - added by birgire 4 years ago.
39494.3.diff (8.5 KB) - added by birgire 4 years ago.
39494.diff (9.4 KB) - added by dlh 8 months ago.
39494.4.diff (9.4 KB) - added by dlh 8 months ago.

Download all attachments as: .zip

Change History (47)

#1 @jason_the_adams
5 years ago

  • Type changed from defect (bug) to enhancement

#2 follow-up: @jnylen0
5 years 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
5 years 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
5 years 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.


4 years ago

#6 @jbpaul17
4 years 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
4 years 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.


4 years ago

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


4 years ago

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


4 years ago

#11 @jbpaul17
4 years ago

  • Keywords needs-testing added; needs-patch removed

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


4 years ago

#13 @jbpaul17
4 years ago

  • Milestone changed from 4.8.1 to Future Release

Punting to Future Release per today's bug scrub.

#14 @Krstarica
4 years ago

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

#15 @jason_the_adams
4 years 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
4 years 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
4 years 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
4 years ago

#18 in reply to: ↑ 7 ; follow-up: @birgire
4 years 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
4 years 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
4 years ago

#20 @birgire
4 years 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
4 years 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
4 years 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.


4 years ago

#24 follow-up: @jason_the_adams
4 years 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
4 years 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.

#26 @Krstarica
18 months ago

Still having hard-coded:

'include_children' => false


#27 @stewei
13 months ago

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?

#28 @TimothyBlynJacobs
11 months ago

  • Keywords needs-refresh needs-unit-tests added

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

#29 @dlh
11 months ago

Sure! I could give this a shot for 5.7.

#30 @TimothyBlynJacobs
11 months ago

  • Milestone changed from Future Release to 5.7
  • Owner set to dlh
  • Status changed from new to assigned

That'd be great! Milestoned and assigned.

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


9 months ago

@dlh
8 months ago

@dlh
8 months ago

#32 @dlh
8 months ago

  • Keywords has-patch has-unit-tests added; needs-refresh needs-unit-tests removed

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.

#33 @TimothyBlynJacobs
8 months ago

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.

#35 @dlh
8 months ago

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?

#36 @TimothyBlynJacobs
8 months ago

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?

#37 @prbot
8 months ago

dlh01 commented on PR #923:

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

#38 @prbot
8 months ago

TimothyBJacobs commented on PR #923:

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.

#39 @prbot
8 months ago

TimothyBJacobs commented on PR #923:

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?

#40 @prbot
8 months ago

dlh01 commented on PR #923:

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

#41 @TimothyBlynJacobs
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.