WP_Term_Query cache problem #2756
Conversation
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
…s-develop into fix/child_of_error
| @@ -770,7 +770,7 @@ public function get_terms() { | |||
| // $args can be anything. Only use the args defined in defaults to compute the key. | |||
| $cache_args = wp_array_slice_assoc( $args, array_keys( $this->query_var_defaults ) ); | |||
|
|
|||
| unset( $cache_args['pad_counts'], $cache_args['update_term_meta_cache'] ); | |||
| unset( $cache_args['update_term_meta_cache'] ); | |||
There was a problem hiding this comment.
Padding counts changes the response, so we have to have a different cache.
| @@ -770,7 +770,7 @@ public function get_terms() { | |||
| // $args can be anything. Only use the args defined in defaults to compute the key. | |||
| $cache_args = wp_array_slice_assoc( $args, array_keys( $this->query_var_defaults ) ); | |||
|
|
|||
| unset( $cache_args['pad_counts'], $cache_args['update_term_meta_cache'] ); | |||
| unset( $cache_args['update_term_meta_cache'] ); | |||
There was a problem hiding this comment.
Padding counts changes the response, so we have to have a different cache.
| /* | ||
| * When querying for terms connected to objects, we may get | ||
| * duplicate results. The duplicates should be preserved if | ||
| * `$fields` is 'all_with_object_id', but should otherwise be | ||
| * removed. | ||
| */ | ||
| if ( ! empty( $args['object_ids'] ) && 'all_with_object_id' !== $_fields ) { | ||
| $_tt_ids = array(); | ||
| $_terms = array(); | ||
| foreach ( $terms as $term ) { | ||
| if ( isset( $_tt_ids[ $term->term_id ] ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| $_tt_ids[ $term->term_id ] = 1; | ||
| $_terms[] = $term; | ||
| } | ||
|
|
||
| $terms = $_terms; | ||
| } |
There was a problem hiding this comment.
This code no longer does anything, so should be removed.
| /* | ||
| * When querying for terms connected to objects, we may get | ||
| * duplicate results. The duplicates should be preserved if | ||
| * `$fields` is 'all_with_object_id', but should otherwise be | ||
| * removed. | ||
| */ | ||
| if ( ! empty( $args['object_ids'] ) && 'all_with_object_id' !== $_fields ) { | ||
| $_tt_ids = array(); | ||
| $_terms = array(); | ||
| foreach ( $terms as $term ) { | ||
| if ( isset( $_tt_ids[ $term->term_id ] ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| $_tt_ids[ $term->term_id ] = 1; | ||
| $_terms[] = $term; | ||
| } | ||
|
|
||
| $terms = $_terms; | ||
| } |
There was a problem hiding this comment.
This code no longer does anything, so should be removed.
|
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 |
There was a problem hiding this comment.
@spacedmonkey Thanks for the explanation, it really helped.
Just one note below about triggering a cache flush on update. The current revision ID on SVN is 53479 so you can use that as the DB version for now but it is usually updated on commit to the current SVN revision.
| @@ -770,7 +770,7 @@ public function get_terms() { | |||
| // $args can be anything. Only use the args defined in defaults to compute the key. | |||
| $cache_args = wp_array_slice_assoc( $args, array_keys( $this->query_var_defaults ) ); | |||
|
|
|||
| unset( $cache_args['pad_counts'], $cache_args['update_term_meta_cache'] ); | |||
| unset( $cache_args['update_term_meta_cache'] ); | |||
There was a problem hiding this comment.
It looks like this changes the cache key & format of the cached data. To be safe the global cache should be flushed during update.
Changing the database version will cause that to happen, no update function is required.
wordpress-develop/src/wp-includes/version.php
Lines 25 to 26 in ae9e0bb
wordpress-develop/src/wp-admin/includes/upgrade.php
Lines 663 to 667 in 2fcc008
The chances of a collision a really slim.
There was a problem hiding this comment.
Unsetting this, will change the cache key. Meaning caches will be invalidated. Meaning we should not have to flush the cache.
|
Committed 6d89ea8 |
Trac ticket: https://core.trac.wordpress.org/ticket/55837
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.