WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 6 days ago

Last modified 39 hours ago

#50225 closed defect (bug) (fixed)

get_edit_term_link can technically accept also WP_Term and object for $term_id

Reported by: david.binda Owned by: audrasjb
Milestone: 5.9 Priority: high
Severity: normal Version:
Component: Taxonomy Keywords: has-patch has-unit-tests needs-dev-note needs-docs
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 19 months ago.
50225.1.diff (8.3 KB) - added by Hareesh Pillai 13 months ago.
Refreshed patch. Fixed typo.
50225.2.diff (9.6 KB) - added by audrasjb 10 months ago.
DocBlock changes

Download all attachments as: .zip

Change History (45)

@david.binda
19 months ago

#1 @SergeyBiryukov
19 months ago

  • Component changed from General to Taxonomy

#2 @SergeyBiryukov
19 months ago

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

#3 @SergeyBiryukov
16 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
14 months ago

  • Keywords has-patch added

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


13 months ago

@Hareesh Pillai
13 months ago

Refreshed patch. Fixed typo.

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


13 months ago

#8 @hellofromTonya
13 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
10 months ago

DocBlock changes

#9 @audrasjb
10 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.


10 months ago

#11 @johnbillion
10 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.


10 months ago

  • Keywords has-unit-tests added

#13 @peterwilsoncc
10 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.


9 months ago

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


6 months ago

#17 @chaion07
6 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
6 months 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.

#19 @hellofromTonya
3 weeks ago

  • Keywords commit added

Upgraded and improved the unit tests. Then manually tested. No regressions. Works the same in 5.8 as it does with this patch. No errors/notices/warnings in the error or debug log.

Ready for commit.

#20 @hellofromTonya
2 weeks ago

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

In 52180:

Taxonomy: Allow get_*_*_link() and edit_term_link() functions to accept a term ID, WP_Term, or term object.

get_term() accepts a term ID, instance of WP_Term, or an object (i.e. stdClass as a result of a db query). Functions that use get_term() also now allow for the same data types.

Why? For consistency, removing extra processing code in consuming functions, and performance.

Functions changed in this commit are:

  • get_category_feed_link()
  • get_term_feed_link()
  • get_tag_feed_link()
  • get_edit_tag_link()
  • get_edit_term_link()
  • edit_term_link()

For each of consumer of these functions, changes to pass the object instead of the term ID.

Includes unit/integration tests for test coverage of these changes.

Follow-up to [6365], [9136], [9340], [14711], [15792], [15800], [18827], [32606], [36646], [37252].

Props davidbinda, johnbillion, peterwilsoncc, hellofromTonya, sergeybiryukov, mista-flo, hareesh-pillai, audrasjb, jeffpaul, chaion07.
Fixes #50225.

#22 @hellofromTonya
2 weeks ago

Thank you everyone for contributing! The patch has been committed and will ship with 5.9 Beta 1 today.

#23 @hellofromTonya
2 weeks ago

  • Keywords needs-dev-note needs-docs added

Docs will need to be updated for these functions. May be worth a mention in Misc dev note.

#24 @hellofromTonya
2 weeks ago

In 52188:

Formatting: Add additional support for single and nestable tags in force_balance_tags().

Adds track and wbr support for single tags.

Adds article, aside, details, figure, and section for nestable tags.

Updates tests.

Follow-up to [5805], [21828], [45929].

Props glendaviesnz, costdev, talldanwp, ramonopoly, sergeybiryukov.
Fixes #50225.

#25 @hellofromTonya
2 weeks ago

Whoopsie, [52188] is not for this ticket. It's for #54130. Moving it there.

This ticket was mentioned in PR #1945 on WordPress/wordpress-develop by Hug0-Drelon.


8 days ago

## Problem

Backward compatibility is broken since WordPress 5.9-alpha because get_term_feed_link() doesn't honor the $taxonomy parameter anymore. See https://github.com/WordPress/wordpress-develop/blob/a8485376d27b5b3348a68963f6fd38200b6e64cf/src/wp-includes/link-template.php#L925-L926

For instance, given a post tag term, get_term_feed_link( $term_id, 'post_tag' ) returns false now.

## Changes

Don't use category as default taxonomy when passing a term ID.
As the WP_Term object is retrieved later, use this to get the taxonomy.

Trac ticket: https://core.trac.wordpress.org/ticket/50225

#27 @hugod
8 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi,

In my opinion, the way get_term_feed_link() has been modified brokes backward compatibility.
I described it in my PR https://github.com/WordPress/wordpress-develop/pull/1945.

Last edited 8 days ago by hugod (previous) (diff)

#28 @audrasjb
8 days ago

  • Keywords commit removed
  • Priority changed from normal to high

#29 follow-up: @audrasjb
8 days ago

  • Owner changed from hellofromTonya to audrasjb
  • Status changed from reopened to reviewing

Thanks for reopening this @hugod, and for the PR. Self-assigning for proper review/test.

#30 in reply to: ↑ 29 @hugod
8 days ago

Replying to audrasjb:

Thanks for reopening this @hugod, and for the PR. Self-assigning for proper review/test.

Hi @audrasjb! Do you prefer I upload a patch here instead of a PR on GitHub ?

#31 @audrasjb
7 days ago

@hugod a PR is fine, thank you :)

#32 @audrasjb
7 days ago

  • Keywords commit added

Thanks @hugod, the PR looks good to me, marking this for commit.

#33 @hugod
7 days ago

Thanks for the review @audrasjb !

#34 @audrasjb
7 days ago

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

In 52255:

Taxonomy: Use WP_Term object to retrieve the taxonomy in get_term_feed_link().

This change fixes a backward compatibility issue introduced in [52180] where get_term_feed_link() did not honor the $taxonomy parameter anymore. Rather than using the default category taxonomy when passing a term ID in get_term_feed_link(), use the WP_Term object to get the taxonomy.

Follow-up to [52180].

Props hugod.
Fixes #50225.

#36 @dlh
7 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm sorry to reopen this again, but [52255] retrieves the $term->taxonomy property before the type safety check on $term:

$taxonomy = $term->taxonomy;

if ( empty( $term ) || is_wp_error( $term ) ) {
	return false;
}

Wouldn't it be preferable to reverse these statements?

Also, the documentation for $taxonomy:

Defaults to 'category' if term ID or non WP_Term object is passed.

seems to no longer be accurate, since $taxonomy is now passed as-is to get_term() regardless of the type of $term.

#38 @audrasjb
7 days ago

  • Keywords commit removed

Thanks for spotting those two issues @dlh!
I think they are fixed in the above PR. I'd appreciate second pair of eyes on this :)

#39 @dlh
7 days ago

That PR looks good to me with respect to the two items I noticed, but other folks who have been following the effort in this ticket as a whole might also want to take a look.

#40 @audrasjb
6 days ago

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

In 52258:

Taxonomy: Check $term object before using it in get_term_feed_link().

This change performs the type safety check on $term before it is used to retrieve the taxonomy in get_term_feed_link(). It also updates the related DocBlock after [52255].

Follow up to [52255].

Props dlh.
Fixes #50225.

#42 @SergeyBiryukov
39 hours ago

In 52305:

Docs: Fix typo in some get_edit_term_link() test DocBlocks.

Follow-up to [52180], [52255].

See #50225, #53399.

Note: See TracTickets for help on using tickets.