Make WordPress Core

Opened 21 months ago

Closed 20 months ago

Last modified 20 months ago

#55837 closed defect (bug) (fixed)

WP_Term_Query cache problem

Reported by: denishua's profile denishua Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0.1 Priority: normal
Severity: normal Version: 6.0
Component: Taxonomy Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description

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");

Change History (35)

#1 @denishua
21 months ago

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

#2 @oztaser
21 months ago

  • Component changed from General to Taxonomy

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' );

#3 @peterwilsoncc
21 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.0.1

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.

This ticket was mentioned in PR #2756 on WordPress/wordpress-develop by spacedmonkey.


21 months ago
#4

  • Keywords has-patch added; needs-patch removed

#5 follow-up: @spacedmonkey
21 months ago

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.

#6 @SergeyBiryukov
21 months ago

#55858 was marked as a duplicate.

#7 @SergeyBiryukov
21 months ago

#55834 was marked as a duplicate.

#8 @SergeyBiryukov
21 months ago

#55834 was marked as a duplicate.

#9 @georgestephanis
21 months ago

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.

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

#10 in reply to: ↑ 5 @denishua
21 months ago

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.

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

#11 follow-up: @spacedmonkey
21 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

#12 @spacedmonkey
21 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#13 @SergeyBiryukov
21 months ago

#55887 was marked as a duplicate.

#14 in reply to: ↑ 11 @jnz31
21 months ago

  • Resolution set to worksforme
  • Status changed from assigned to closed

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)

#15 @knutsp
21 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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

spacedmonkey commented on PR #2756:


21 months ago
#16

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

spacedmonkey commented on PR #2756:


21 months ago
#17

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

spacedmonkey commented on PR #2756:


21 months ago
#18

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

#19 @mukesh27
21 months ago

Is this #29894 ticket is related to this issue?

#20 @spacedmonkey
21 months ago

  • Keywords commit added

I am going to commit this.

#21 @spacedmonkey
21 months ago

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

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.

#22 follow-up: @spacedmonkey
21 months ago

  • Keywords commit removed

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 ?

#24 in reply to: ↑ 22 @SergeyBiryukov
21 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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 :)

#25 @spacedmonkey
21 months ago

  • Owner changed from spacedmonkey to SergeyBiryukov
  • Status changed from reopened to assigned

Reassigned for backporting.

#26 @xParham
21 months ago

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?

#27 @davelo
20 months ago

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!

Last edited 20 months ago by davelo (previous) (diff)

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


20 months ago

#29 @SergeyBiryukov
20 months ago

#56055 was marked as a duplicate.

#30 @spacedmonkey
20 months ago

@SergeyBiryukov Has this been backported yet?

#31 @SergeyBiryukov
20 months ago

Will backport in a bit :)

#32 @SergeyBiryukov
20 months ago

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

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

#33 @peterwilsoncc
20 months ago

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.

#34 @peterwilsoncc
20 months ago

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

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

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


20 months ago

Note: See TracTickets for help on using tickets.