WordPress.org

Make WordPress Core

Opened 15 months ago

Last modified 8 weeks 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: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch has-unit-tests
Focuses: performance Cc:

Description

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 (3)

50225.diff (8.4 KB) - added by david.binda 15 months ago.
50225.1.diff (8.3 KB) - added by Hareesh Pillai 9 months ago.
Refreshed patch. Fixed typo.
50225.2.diff (9.6 KB) - added by audrasjb 6 months ago.
DocBlock changes

Download all attachments as: .zip

Change History (21)

@david.binda
15 months ago

#1 @SergeyBiryukov
15 months ago

  • Component changed from General to Taxonomy

#2 @SergeyBiryukov
15 months ago

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

#3 @SergeyBiryukov
12 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
10 months ago

  • Keywords has-patch added

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


9 months ago

@Hareesh Pillai
9 months ago

Refreshed patch. Fixed typo.

#6 @johnbillion
9 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.


9 months ago

#8 @hellofromTonya
9 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.

@audrasjb
6 months ago

DocBlock changes

#9 @audrasjb
6 months ago

  • Keywords needs-refresh removed

The last patch adds some DocBlocks/coding standards issues.

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


6 months ago

#11 @johnbillion
6 months ago

It looks like this patch still changes the type of the value passed to the filters as mentioned in comment 6. Can you check that please?

This ticket was mentioned in PR #1021 on WordPress/wordpress-develop by peterwilsoncc.


6 months ago

  • Keywords has-unit-tests added

#13 @peterwilsoncc
6 months ago

In my linked pull request:

  • Added a bunch of tests for edit_term_link(), get_edit_term_link()
  • Tests ensure an integer is sent to the filter in the hooks get_edit_term_link and edit_term_link
  • Tests ensure a WP_Term object is sent to the filter in term_link
  • Fixed filters per comment 6 and comment 11
  • Changed edit_term_link() to also accept a term ID or object in line with get_edit_term_link() -- it seems potentially confusing for the equivalent parameters to differ
  • There was accidental revert of an earlier commit in the patch, removed that

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


5 months ago

#15 @hellofromTonya
5 months ago

  • Milestone changed from 5.7 to 5.8

With 5.7 RC1 minutes away, ran out of time for this one. Moving to 5.8 for it to go through a beta cycle.

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


2 months ago

#17 @chaion07
2 months ago

Thanks to @davidbinda for reporting this issue. We reviewed this ticket during a recent [bug-scrub session]https://wordpress.slack.com/archives/C02RQBWTW/p1623096669357200. With Beta 1 coming in a day, checking with @SergeyBiryukov to kindly draw some attention to this. Thank you! Otherwise it is headed for a punt to Future Release if that patch isn't solid enough to land in time. Thanks

Props to @jeffpaul

#18 @hellofromTonya
8 weeks ago

  • Milestone changed from 5.8 to 5.9
  • Owner changed from SergeyBiryukov to hellofromTonya

Today is 5.8 Beta 1. Ran out of time for this ticket to land.

Though this ticket has been punted several times, PR 50225 looks close to being ready for testing. Punting to 5.9 (instead of Future Release).

Hey @SergeyBiryukov reassigning ownership to me and added to my code/test review and testing queue.

Note: See TracTickets for help on using tickets.