WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 weeks ago

#41246 assigned enhancement

Filter to short circuit term query 'terms_pre_query'

Reported by: jarocks Owned by: adamsilverstein
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.6
Component: Taxonomy Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

It would be nice to be able to effectively short-circuit a terms query so that you could return results from an external data source (WP_Query already has a filter like this in the form of posts_pre_query). My patch would add a similar filter terms_pre_query into the ::parse_query() method of the WP_Term_Query class.

This is more a proof of concept than anything else, I'm not sure if it makes sense to leave the burden of handling term caching as well as the hide_empty and the pad_counts parameters on the callback function. Hopefully some more experienced contributors will have some better insight into that.

Attachments (3)

41246.diff (3.9 KB) - added by jarocks 2 years ago.
41246.2.diff (1.2 KB) - added by spacedmonkey 6 months ago.
41246.3.diff (2.6 KB) - added by spacedmonkey 3 weeks ago.

Download all attachments as: .zip

Change History (11)

@jarocks
2 years ago

#1 @jarocks
2 years ago

  • Summary changed from Filter to short circuit term query to Filter to short circuit term query 'terms_pre_query'

#2 @boonebgorges
2 years ago

  • Milestone changed from Awaiting Review to Future Release

Hi @jarocks - Thanks for the ticket! I like the idea.

For reference, #36687 was the ticket that introduced posts_pre_query.

I'm unsure off the top of my head how best to handle all the post-query-processing that happens in WP_Term_Query. By way of comparison, WP_Query continues to perform all of its own post_status post-processing on posts provided by external data sources. We want to try to strike a balance between total customizability on the one hand, and giving developers all the conveniences of core queries on the other.

As a way forward, could I suggest that you write a sample implementation? It's fine to use dummy data, of course. But I think it would be very helpful to get a sense of how the filter would work best. Ideally, the dummy data would be hierarchical, so we can get a sense of how count padding etc would work in practice. Since the data doesn't like in WP, it's likely that the callback function will *have* to handle all of this stuff, but it'll be a helpful exercise to see this in practice.

#3 @spacedmonkey
6 months ago

  • Keywords has-patch added
  • Version set to 4.6

Updated the patch. 41246.2.diff to be more similar to #44169.

Looping @adamsilverstein as he owned that.

#5 @adamsilverstein
6 months ago

Thanks @spacedmonkey this looks good to me.

@boonebgorges when you have a chance, can you review 41246.2.diff and the code path in get_terms. As. This looks good to me, however I'm uncertain whether we need to be concerned about priming caches with returned data or updating term counts.

#6 @boonebgorges
6 months ago

Thanks for the ping, @adamsilverstein . 41246.2.diff seems OK to me. Regarding caching, I'd say that core should definitely not be caching the results of short-circuited queries - this kind of "pre" filter hands over all such responsibilities to the plugin.

I'm not sure what you mean by "updating term counts". Do you mean _update_post_term_count() etc? These are not term counts but *object-in-term* counts, and they don't touch the WP_Term_Query results. (They do depend on the term_taxonomy_id from WP's database tables, but again, any plugin using this filter is going to have to figure out how to reconcile this information on its own.) Or by "term counts" do you mean get_terms( fields=count )? In this case too, I'd say that the terms_pre_query callback will just have to be smart enough to detect 'count' queries and return the right results - not much that we can do in core.

@spacedmonkey
3 weeks ago

#7 @spacedmonkey
3 weeks ago

  • Keywords has-unit-tests added
  • Milestone changed from Future Release to 5.3
  • Owner set to adamsilverstein
  • Status changed from new to assigned

Update the patch with 41246.3.diff. This is basically a copy and paste from #45749.

I have assigned this to @adamsilverstein to review. But I would also like feedback from @boonebgorges .

#8 @boonebgorges
2 weeks ago

  • Keywords commit added

Approach in 41246.3.diff looks good to me.

Note: See TracTickets for help on using tickets.