WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 21 months ago

Last modified 17 months ago

#42771 closed defect (bug) (fixed)

WP_Term::get_instance() regression for non-category terms queried with 'category' taxonomy

Reported by: markjaquith Owned by:
Milestone: 4.9.2 Priority: high
Severity: major Version: 4.9
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

WP_Term::get_instance() changed in 4.9 which caused a lot of other issues, probably including:

#42717
#42691
#42605
#42732

Other reports here:
https://wordpress.org/support/topic/category-description-broken-after-4-9-update/
https://wordpress.org/support/topic/4-9-get_category_link-not-working/

What seems to be happening is this:

Some old category function gets called on a term_id that does not exist as a category (let's say 123).

e.g. get_category_link( 123 );.

This ends up calling WP_Term::get_instance( 123, 'category' );

This worked before 4.9, because when the taxonomy failed to match...

if ( ! $_term || ( $taxonomy && $taxonomy !== $_term->taxonomy ) ) {

...$_term would be left alone.

But in 4.9, this happens:

// If there isn't a cached version, hit the database.
if ( ! $_term || ( $taxonomy && $taxonomy !== $_term->taxonomy ) ) {
    // Any term found in the cache is not a match, so don't use it.
    $_term = false;

The query runs, but it won't find a taxonomy match when iterating the results, and thus will return false.

The old behavior seems... undesirable. You're querying a category term and potentially getting back a term with another taxonomy.

We could fix most of the cases in the wild by exempting category from the $_term = false; bailout. But then we're just perpetuating category being a special case.

Attachments (4)

42771.diff (461 bytes) - added by boonebgorges 21 months ago.
42771-tests.diff (1.4 KB) - added by boonebgorges 21 months ago.
42771-tag-specific.diff (421 bytes) - added by juiiee8487 21 months ago.
Created patch for tag specific function.
42771.1-tag-specific.diff (572 bytes) - added by juiiee8487 21 months ago.
Created patch for tag specific function to make more simplify.

Download all attachments as: .zip

Change History (23)

#1 @markjaquith
21 months ago

#40671 was that prompted this code change. The change was intentional, but some of the effects were not explicitly discussed.

Maybe we could have a doing_it_wrong() for people who are calling category functions on non-category term IDs.

#2 @boonebgorges
21 months ago

  • Milestone changed from Awaiting Review to 4.9.2

Thanks for opening the ticket.

#42717 and #46205 are definitely related, and #42691 probably is. #42732 appears to be unrelated; see https://core.trac.wordpress.org/changeset/40918.

If I'm understanding correctly - and setting aside the regression for a moment - [40979] fixes an inconsistency in the way taxonomy mismatches were previously handled. On an empty cache, a database query would take place, which would fetch items from all taxonomies; if a term in the proper tax wasn't found, get_instance() would return false. But if the cache were populated, it'd return the term. After [40979], the behavior is consistent regardless of the cache state. See the attached tests, and try running them with [40979] rolled back. In cases where a regression is reported, it just so happens that the reporters were testing against a case where the cache in question had already been populated by some other query on the page.

I wonder if we can address the regression by changing the calls to get_term() so that they don't specify a taxonomy. See the attached patch for get_category_link(). This too is technically a regression from pre-4.9 behavior (see the comment above about caching). But it'll probably fix most problems in the wild, leaving only the category template functions as the exceptions, rather than hardcoding an exception deep in the API.

@markjaquith Your wisdom here would be appreciated :-D

@boonebgorges
21 months ago

#3 follow-up: @dd32
21 months ago

  • Keywords has-patch added

42771.diff seems like a good direction to me - just make the category-specific functions not care if it's an actual category or not. We could probably do the same to the tag-specific functions if any of those exist.

@juiiee8487
21 months ago

Created patch for tag specific function.

#4 in reply to: ↑ 3 @juiiee8487
21 months ago

Replying to dd32:

42771.diff seems like a good direction to me - just make the category-specific functions not care if it's an actual category or not. We could probably do the same to the tag-specific functions if any of those exist.

I have created patch 42771-tag-specific.diff for get_tag_link() based on @boonebgorges's 42771.diff

I hope that will be helpful.

#5 follow-up: @markjaquith
21 months ago

I think this approach could work nicely! Solves the weird cases in the wild without introducing special case handling to our generic terms functions.

@juiiee8487 I think we can simplify that even further... get_tag_link() could just call get_category_link() since we'll be removing the only difference between the two functions!

@juiiee8487
21 months ago

Created patch for tag specific function to make more simplify.

#6 in reply to: ↑ 5 @juiiee8487
21 months ago

Replying to markjaquith:

I think this approach could work nicely! Solves the weird cases in the wild without introducing special case handling to our generic terms functions.

@juiiee8487 I think we can simplify that even further... get_tag_link() could just call get_category_link() since we'll be removing the only difference between the two functions!

Yes. You are right. How about this 42771.1-tag-specific.diff?

#7 @boonebgorges
21 months ago

In 42364:

Don't do a strict taxonomy check in get_category_link().

Prior to version 4.9, a quirk in the implementation of get_term() caused
get_category_link( 123 ) to fetch the taxonomy archive link for term 123
even if 123 is not in the 'category' taxonomy. The quirk was fixed in [40979];
see #40671. This bugfix introduced a regression for theme authors who were
expecting the old behavior.

By lifting the 'category' restriction, we allow the template function to work
in the old way.

Fixes #42717. See #42771.

#8 @boonebgorges
21 months ago

In 42365:

Fix incorrect test names from [42364].

See #42771.

#9 @boonebgorges
21 months ago

In 42366:

Fix regression in get_tag_link() since 4.9.

See [42364] for description of the problem.

Because get_category_link() is now totally taxonomy-agnostic, get_tag_link()
can become a simple wrapper.

Props juiiee8487, markjaquith.
See #42771.

#10 @boonebgorges
21 months ago

In 42367:

get_the_category_by_ID() should be taxonomy-agnostic.

Prior to 4.9, this function was accidentally taxonomy-agnostic in most cases.
The fix in [40979] caused a regression in this function. For backward
compatibility, we make it explicit that the query is by ID only.

See #42771.

#11 @boonebgorges
21 months ago

In 42368:

category_description() should be taxonomy-agnostic.

This change reinstates the previous de facto behavior of category_description().
See [40979], [42364]. Because term_description() no longer passes $taxonomy to
get_term_field(), the parameter is no longer needed and has been deprecated.

Fixes #42605. See #42771.

#12 @boonebgorges
21 months ago

In 42369:

Don't do a strict taxonomy check in get_category_link().

Prior to version 4.9, a quirk in the implementation of get_term() caused
get_category_link( 123 ) to fetch the taxonomy archive link for term 123
even if 123 is not in the 'category' taxonomy. The quirk was fixed in [40979];
see #40671. This bugfix introduced a regression for theme authors who were
expecting the old behavior.

By lifting the 'category' restriction, we allow the template function to work
in the old way.

Merges [42364], [42365] to the 4.9 branch.

Fixes #42717. See #42771.

#13 @boonebgorges
21 months ago

In 42370:

Fix regression in get_tag_link() since 4.9.

See [42364] for description of the problem.

Because get_category_link() is now totally taxonomy-agnostic, get_tag_link()
can become a simple wrapper.

Merges [42366] to the 4.9 branch.

Props juiiee8487, markjaquith.
See #42771.

#14 @boonebgorges
21 months ago

In 42371:

get_the_category_by_ID() should be taxonomy-agnostic.

Prior to 4.9, this function was accidentally taxonomy-agnostic in most cases.
The fix in [40979] caused a regression in this function. For backward
compatibility, we make it explicit that the query is by ID only.

Merges [42367] to the 4.9 branch.

See #42771.

#15 @boonebgorges
21 months ago

In 42372:

category_description() should be taxonomy-agnostic.

This change reinstates the previous de facto behavior of category_description().
See [40979], [42364]. Because term_description() no longer passes $taxonomy to
get_term_field(), the parameter is no longer needed and has been deprecated.

Merges [42368] to the 4.9 branch.

Fixes #42605. See #42771.

#16 @boonebgorges
21 months ago

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

The following functions have been made taxonomy-agnostic:

  • get_category_link() #42717
  • get_tag_link() #42756
  • get_the_category_by_ID()
  • category_description() #42605

I've done a review of other category functions, and I don't see any more obvious candidates for the same treatment.

It's worth noting that there's still the possibility for regressions in cases where people are passing a mismatched ID + taxonomy into *any* term function, since previously these queries would sometimes have worked, while now they won't. This seems like less of an important regression, since these sorts of usages are clearly a result of user error (or some sort of shared-term legacy problem).

So, I'm going to close this ticket, but let's reopen if we come across more similar regressions in the wild.

Last edited 21 months ago by boonebgorges (previous) (diff)

#17 @SergeyBiryukov
20 months ago

#42756 was marked as a duplicate.

This ticket was mentioned in Slack in #docs by chetan200891. View the logs.


19 months ago

#19 @SergeyBiryukov
17 months ago

In 42890:

Taxonomy: In category_description(), don't pass the $taxonomy parameter to term_description().

The parameter was deprecated in [42368] and is now unused.

Props chetan200891.
Fixes #43381. See #42771.

Note: See TracTickets for help on using tickets.