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

[markjaquith]

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.

[markjaquith]

#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.

[boonebgorges]

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

[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.

[juiiee8487]

Created patch for tag specific function.

[juiiee8487]

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.

[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!

[juiiee8487]

Created patch for tag specific function to make more simplify.

[juiiee8487]

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?

[boonebgorges]

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.

[boonebgorges]

In 42365:

Fix incorrect test names from [42364].

See #42771.

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[SergeyBiryukov]

#42756 was marked as a duplicate.

[SergeyBiryukov]

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.