WP_Term_Query cache problem

[denishua]

when use child_of parameter in WP_Term_Query,
it return the right result first time,
it return all terms the second times when hit cache.

my test code:

wp_list_categories("child_of=2&depth=1&hide_empty=0");

[denishua]

I think the WP_Term_Query get_terms method only handle $term_objects, but forgot $terms variable when deal with child_of parameter.

the fast way fix the problem is move line 891 before line 890

[oztaser]

Hi there,

I also noticed this issue. Here is my mu-plugin to see the difference between cached and non-cached data:

<?php
namespace NefisYemekTarifleri\TERM_CACHE_TEST;

function init() {
        $last_changed = microtime();
        wp_cache_set( 'last_changed', $last_changed, 'terms' );

        $args = array(
                'taxonomy' => 'category',
                'child_of' => 2219, // some category id.
        );

        $terms = get_terms( $args );
        $terms = wp_list_pluck( $terms, 'name' );

        $terms2 = get_terms( $args );
        $terms2 =  wp_list_pluck( $terms2, 'name' );

        echo count( $terms );
        var_dump( $terms );

        echo '<hr />';

        echo count( $terms2 );
        var_dump( $terms2 );
        //die();
}
add_action( 'init', __NAMESPACE__ . '\\init' );

[peterwilsoncc]

Thanks for your report @denishua, I am able to reproduce have confirmed the bug was introduced in WordPress 6.0. I've placed this on the 6.0.1 milestone for review.

@spacedmonkey do you have any bandwidth to look in to this? I'll be on leave next week, returning June 6 2022, so won't be available for any code review you may require.

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

[spacedmonkey]

Thanks for the ping @peterwilsoncc !

I have put together a patch. This does a number of things.

• Fixes the core issue, that was highlighted here.
• Removes confusing and unneeded code.
• Improves the cache value, so it now only stores an array of ids, in all but two caches ( count and object with ids ).

This will also improve memory usage on every page, which is another side benefit.
Once someone has proved that this fixes the issue, I will add some unit tests.

[SergeyBiryukov]

#55858 was marked as a duplicate.

[SergeyBiryukov]

#55834 was marked as a duplicate.

[SergeyBiryukov]

#55834 was marked as a duplicate.

[georgestephanis]

If it's of use to anyone, we were hitting some issues with this and @spencercameron threw this together as a temporary fix until it's resolved in 6.0.1 and it seems to mitigate the issue in our testing on one site so far -- (we're working on rolling it out to a few others and vetting it there as well to confirm more broadly)

​https://gist.github.com/pippercameron/3294971e9c0810ba491a68278d0ceb10

Full disclaimers! Test it first in a dev environment! No warranties, but may help mitigate the issue.

There may be future revisions on it, but didn't want to sit on it if it can help anyone else.

[denishua]

I think is better to cache terms immediately when get from db.

<?php
$terms = $wpdb->get_results( $this->request );

otherwise if

$args['pad_counts']

is true and

 $args['number']

is set, the pad_counts of term will be different.

Replying to spacedmonkey:

Thanks for the ping @peterwilsoncc !

I have put together a patch. This does a number of things.

• Fixes the core issue, that was highlighted here.
• Removes confusing and unneeded code.
• Improves the cache value, so it now only stores an array of ids, in all but two caches ( count and object with ids ).

This will also improve memory usage on every page, which is another side benefit.
Once someone has proved that this fixes the issue, I will add some unit tests.

[spacedmonkey]

@denishua I have a patch that will fix both issues. Any chance you could test my PR?

[SergeyBiryukov]

#55887 was marked as a duplicate.

[jnz31]

Replying to spacedmonkey:

@denishua I have a patch that will fix both issues. Any chance you could test my PR?

thanks @spacedmonkey
i applied your patch to my live website and the categories are back up and running again. before that, my code

$args = [
  'taxonomy' => 'product_cat',
  'title_li' => '',
  'exclude' => [123, 456],
  'orderby' => 'menu_order',
  'echo' => false,
];
echo '<ul class="collection-list">' . wp_list_categories($args) . '</ul>';

returned: no categories
(i updated to wp 6.0 yesterday, before i had 5.9.3 running and the product_cats were working fine)

[knutsp]

Ticket will be closed when patch committed to trunk and/or target version.

@peterwilsoncc

The biggest change here, is that before there were two variables, $term_objects and $terms. These variables basically did the same thing but depending on what params were passed, would result in different values. This PR, ensure that if pad_counts or child_of are passed, that the correct value is returned. It also makes the code a lot easier to read, by removing two variables. It also improves cache value, by only caching an array of ids, if other properties are not needed.

@peterwilsoncc

The biggest change here, is that before there were two variables, $term_objects and $terms. These variables basically did the same thing but depending on what params were passed, would result in different values. This PR, ensure that if pad_counts or child_of are passed, that the correct value is returned. It also makes the code a lot easier to read, by removing two variables. It also improves cache value, by only caching an array of ids, if other properties are not needed.

@peterwilsoncc

The biggest change here, is that before there were two variables, $term_objects and $terms. These variables basically did the same thing but depending on what params were passed, would result in different values. This PR, ensure that if pad_counts or child_of are passed, that the correct value is returned. It also makes the code a lot easier to read, by removing two variables. It also improves cache value, by only caching an array of ids, if other properties are not needed.

[mukesh27]

Is this #29894 ticket is related to this issue?

[spacedmonkey]

I am going to commit this.

[spacedmonkey]

In 53496:

Taxonomy: Fix caching issues in WP_Term_Query class.

Introduced [52836] when passing child_of or pad_counts parameters to get_terms or WP_Term_Query class, the array of terms received by the query, was not correctly cached. This
change simplifies the logic in WP_Term_Query and ensures terms are correctly cached. This change also, improves performance, by only caching an array of term ids where possible.

Props denishua, spacedmonkey, oztaser, peterwilsoncc, SergeyBiryukov, georgestephanis, jnz31, knutsp, mukesh27, costdev.
Fixes #55837.

[spacedmonkey]

The commit [53496] is now in trunk. This needs to backported to 6.0 branch. Any recommendations on how to do this @SergeyBiryukov @desrosj @peterwilsoncc ?

Committed ​https://github.com/WordPress/wordpress-develop/commit/6d89ea8f09cf21ef0d1e1acd6e1b30fba1a72301

[SergeyBiryukov]

Replying to spacedmonkey:

This needs to backported to 6.0 branch. Any recommendations on how to do this?

Reopening for backporting to the 6.0 branch.

The ​Backporting Commits handbook article might be helpful :)

[spacedmonkey]

Reassigned for backporting.

[xParham]

I am running into similar caching issues with WP_Term_Query on 6.0, specifically when using the exclude_tree argument to filter the results. It works fine for the first run, but then all the subsequent runs of the same query return no results, and basically makes some functions unusable. Does this patch already fix the issue with the exclude_tree parameter too? or do I need to create a new bug report?

[davelo]

We have 3 cases around the terms caching introduced in WP6.0, mainly when it's about bigger queries (seems like) on big WooCommerce shops. On daily basis, the subcategories on category pages (of shop page) are disappearing. Mostly on shops that use parent and subcategories, but only check the subcategorie when they add products. So, therefore it seems like the "parent" category is empty, apparently of the way the new caching works (on term id).

A manual recount terms (a setting in WooCommerce) must be done, daily since a few weeks.

Been investigating it for weeks. Finally found this ticket. Hopefully the solution will be rolled out quickly. Thanks!

[SergeyBiryukov]

#56055 was marked as a duplicate.

[spacedmonkey]

@SergeyBiryukov Has this been backported yet?

[SergeyBiryukov]

Will backport in a bit :)

[SergeyBiryukov]

I was puzzled a bit by the $wp_db_version version bump. It looks like the intention was to ​clear the cache, and I see the wp_cache_flush() call in update_core(), but unless I'm missing something, it runs regardless of the $wp_db_version value, followed by some more code​, none of which depends on $wp_db_version. So I'm curious why the version bump is needed :)

If the DB version is bumped, wp_cache_flush() runs again in wp_upgrade() after schema upgrade, but since there's no actual schema upgrade here, it seems that for the purpose of the ticket, this second call would be redundant, as the one in update_core() should be enough.

In a discussion with @peterwilsoncc it was determined that $wp_db_version should still be bumped for the upgrade_600() routine to run, as it was missed in [53011]. In that case, if it's not necessary for the cache clearing here, might be a good idea to do the bump separately, for clarity.

Related: comment:16:ticket:55890

[peterwilsoncc]

I realised why I thought this needed the DB version bump.

While update_core() does flush the cache for sites using the WP Admin upgrader, it's not run for sites using a more advanced deployment: composer, version control or similar.

As sites running a persistent cache are more likely to use such a configuration, including the cache flush on this ticket will be helpful.

I'll add a see reference to the other ticket when backporting.

[peterwilsoncc]

In 53650:

Taxonomy: Fix caching issues in WP_Term_Query class.

Introduced [52836] when passing child_of or pad_counts parameters to get_terms or WP_Term_Query class, the array
of terms received by the query, was not correctly cached. This change simplifies the logic in WP_Term_Query and
ensures terms are correctly cached. This change also, improves performance, by only caching an array of term ids where
possible.

Additionally, the database version bump included in this patch is also required for #55890 to initialize the user count
on single sites.

Props denishua, spacedmonkey, oztaser, peterwilsoncc, SergeyBiryukov, georgestephanis, jnz31, knutsp, mukesh27, costdev,
tharsheblows.
Merges [53496] to the 6.0 branch.
Fixes #55837, #55890.

--This line, and those below, will be ignored--

_M .
M src/wp-includes/class-wp-term-query.php
M src/wp-includes/version.php
M tests/phpunit/tests/term/getTerms.php