Make WordPress Core

Opened 8 months ago

Last modified 2 months ago

#50225 reviewing defect (bug)

get_edit_term_link can technically accept also WP_Term and object for $term_id

Reported by: david.binda Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch needs-refresh
Focuses: performance Cc:


The doc block for get_edit_term_link currently says it can only accept the term_id, but the $term_id param is being passed to the get_term function right away, which, IMHO, technically means it can be anything what the get_term function accepts, which is int, WP_Term, object.

I'm really able to figure out why the documentation limits the param to term_id only. I wonder if I perhaps missed something?

However, the get_term behaves differently when it's being passed different values/types. From the documentation:

(int|WP_Term|object) (Required) If integer, term data will be fetched from the database, or from the cache if available. If stdClass object (as in the results of a database query), will apply filters and return a WP_Term object corresponding to the $term data. If WP_Term, will return $term.

I have noticed that in many places where the code is passing just the term_id to get_edit_term_link, the code might go over some unnecessarily complicated logic, eventually causing some minor inefficiencies.

For instance, the wp_tag_cloud. The function first collects all the terms, which means collecting whole lines of terms entries in database, populating the terms, and then loops over those passing in just the `term_id` to the `get_edit_term_link` (as well as get_term_link, tho).

Which means that the get_edit_term_link calls the get_term just with the term_id, which in turn, collects the very same data from local cache, in ideal situation, despite the data have all been already collected by get_terms and are ready in the get_edit_term_link function.

However, there's also a situation in which passing in just the term_id actually triggers unnecessary SQL queries or prompts some remote cache get requests.

In case the term is not being used elsewhere on the page, and in case a persistent object cache is in use, the get_terms may end early if it was cached previously, there is an early return, only triggering the WP_Term::populate_terms, skipping the populate_term_caches call, which otherwise happens later.

So, the terms are well populated, but they are not in local object cache, and need to be brought from the object cache server, and if not found there, the get_term function, when passed in just the term_id, fetches the data from the database again.

That said, I believe that the possibility of passing in the WP_Term (or object) to get_edit_term_link should be documented, and used by WordPress itself on respective places.

It, IMHO, would also be beneficial, if we could go further, and pass in the WP_Term or object to any similar calls, eg: `get_term_feed_link`, which can in theory also accept the WP_Term object, and which is being called repeatedly from `Walker_Category::start_el`, which is being called from wp_list_categories which previously collected the WP_Term objects via `get_categories` (which is calling the get_terms)

The same goes for WP_List_Table.

I'm attaching a patch, with those adjustments.

Attachments (2)

50225.diff (8.4 KB) - added by david.binda 8 months ago.
50225.1.diff (8.3 KB) - added by Hareesh Pillai 2 months ago.
Refreshed patch. Fixed typo.

Download all attachments as: .zip

Change History (10)

8 months ago

#1 @SergeyBiryukov
8 months ago

  • Component changed from General to Taxonomy

#2 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#3 @SergeyBiryukov
6 months ago

  • Milestone changed from 5.5 to 5.6

Didn't get a chance to review this one in time for 5.5 RC, let's handle this in 5.6.

#4 @Mista-Flo
4 months ago

  • Keywords has-patch added

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

2 months ago

@Hareesh Pillai
2 months ago

Refreshed patch. Fixed typo.

#6 @johnbillion
2 months ago

  • Keywords needs-refresh added

Note that the type of the $term_id parameter that's passed to the filters cannot change. This affects get_edit_tag_link and get_edit_term_link. The parameter needs to remain an integer for backwards compatibility, but a new parameter can be added.

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

2 months ago

#8 @hellofromTonya
2 months ago

  • Milestone changed from 5.6 to 5.7

With 5.6 RC1 tomorrow, it's too late in the cycle. Punting this ticket.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Note: See TracTickets for help on using tickets.