WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 10 months ago

#40661 new enhancement

WP_Term_Query->parse_query() needs filter like 'get_terms'

Reported by: mpol Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

The new class/method WP_Term_Query->parse_query() does not provide a filter like the old function get_terms() does. Using WP_Term_Query directly has thus different functionality from using the old wp_terms function.

Using a plugin like: https://wordpress.org/support/plugin/custom-taxonomy-order-ne/ with a custom term_order field and sorting based on that, gives the default sort based on name. Adding a filter to the method like get_terms, provides backwards compatibility for plugin and theme authors, that are switching to WP_Term_Query directly. This filter gives options to apply a custom order to the list of terms.

Reference on support forum: https://wordpress.org/support/topic/doesnt-reorder-when-using-wp_term_query/

If adding a filter is somehow not wanted by core devs, then I will have to answer to add this filter manually to the code when support requests are coming. To be clear, I would consider that a valid response.

Attachments (4)

wp_term_query_filter.diff (575 bytes) - added by mpol 13 months ago.
Add filter get_terms to WP_Term_Query->parse_query()
40661.diff (3.9 KB) - added by jarocks 11 months ago.
Alternative take on get_terms filter
filter-term-query-results.patch (905 bytes) - added by mpol 11 months ago.
term_query_results filter for get_terms method, including docblock.
filter-term-query-results_2.patch (1.2 KB) - added by mpol 10 months ago.
Another try :). Doesnot contaminate cache.

Download all attachments as: .zip

Change History (12)

@mpol
13 months ago

Add filter get_terms to WP_Term_Query->parse_query()

#1 @mpol
13 months ago

The patch in attachment 1 works for me, but I am definitely no core dev, so you want to look at it critically.

#2 follow-up: @ocean90
12 months ago

  • Keywords has-patch needs-refresh needs-unit-tests added
  • Type changed from defect (bug) to enhancement
  • Version trunk deleted

@mpol Your patch is missing the file name. Also, could you please document the filters?

How do the other ::parse_query() methods handle this case?

@jarocks
11 months ago

Alternative take on get_terms filter

#3 in reply to: ↑ 2 @jarocks
11 months ago

For consistency with the ::parse_query() method in WP_Query, it might make more sense to implement this similarly to the filter posts_pre_query. See 40661.diff for my iteration of a terms_pre_query filter.

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.

Last edited 11 months ago by jarocks (previous) (diff)

#4 @boonebgorges
11 months ago

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

Hi all - @mpol Thanks for the report, and @jarocks thanks for the follow-up patch.

For clarification, this appears to have nothing to do with parse_query(). parse_query(), in WP_Term_Query and in other query classes, is where the arguments passed to the query() method are parsed together with the default arguments and validated. WP_Term_Query::parse_query() does, in fact, have an action available for plugins to intervene: parse_term_query.

What's being requested here is a filter on the results of the query. The get_terms filter still exists, but when WP_Term_Query was introduced, it was decided to leave that filter in the get_terms() wrapper function, for naming consistency.

@jarocks The pre-query filter you've suggested here is interesting, but I don't think it serves the purpose of this ticket. custom-taxonomy-order-ne and similar plugins work by modifying the results of the core query; posts_pre_query is a way of preventing core queries from taking place.

Here's the pattern elsewhere in our query classes:

  • WP_Query runs a filter posts_results on query results
  • WP_Comment_Query runs a filter the_comments on query results
  • WP_User_Query has no results filter, either in the query class or in the get_users() wrapper function

So there's not a great pattern to follow here. term_query_results seems like a reasonable naming convention. What do others think?

#5 @jarocks
11 months ago

@boonebgorges with your comment in mind, I've moved my proposed filter into a separate ticket: 41246

#6 @mpol
11 months ago

Thanks for commenting. Sorry for being confused with the parse_query function, I now see that the request should be about the get_terms function. I can agree with the naming function. I was hoping to reuse a filter name like get_terms or get_the_terms (there are too many already), but the parameters are different, so it will have to be a new name.

I have a better patch prepared, and will attach it. I added the filter right after the get_results wpdb query, because caches get updated right away, and I think the filter should be run before the cache gets updated. It works for me and the usecase of the referenced support topic. Please rework as you see fit.

@mpol
11 months ago

term_query_results filter for get_terms method, including docblock.

#7 @boonebgorges
10 months ago

Thanks, @mpol!

Having the filter placed as in filter-term-query-results.patch means that the cache will be populated with items returned from filters. This seems problematic to me, since an error by a developer could result in incorrectly cached content. Moreover, it means that the filter will run on values coming from a database query *as well as* values coming out of the cache, when the latter will already have been cached on the way into the database. As such, I'd suggest moving the filter so that it runs on what's been pulled from the cache. Does that seem right?

Note that the approach here (especially in light of the preceding paragraph) will probably need revisiting after #37189.

@mpol
10 months ago

Another try :). Doesnot contaminate cache.

#8 @mpol
10 months ago

Ah, interesting :). I didn't know caches only use raw data. I am not sure of the implications of #37189, I will wait what happens with that.

For now I attached a new patch (see above). The filter on line 672 is about getting data from cache, filtering it and returning it. The same filter on line 804 is about fresh data that is being filtered, after it has been added to cache.

Note: See TracTickets for help on using tickets.