WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#13318 closed defect (bug) (fixed)

Bug caching get_terms

Reported by: djudorange Owned by: ryan
Milestone: 3.2 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

I need to use two wp_list_categories with different params which I'll use in filter:

<?php wp_list_categories( array('cible' => 'homme' ) )?>
<?php wp_list_categories( array('cible' => 'trendy' ) )?>

But in taxonomy.php, the function get_terms, the cache work over $default instead of $args.
So, the function wp_list_categories return the same result.

Attachments (6)

taxonomy.diff (729 bytes) - added by djudorange 4 years ago.
the patch
13318.get-terms-custom-arg.diff (730 bytes) - added by mfields 3 years ago.
Adds a recognized arg for "custom" which defaults to boolean false.
13318.get-terms-custom-arg-2.diff (1.8 KB) - added by mfields 3 years ago.
Less hacky approach as suggest by Nacin. Added docs too.
13318.get-terms-custom-arg-3.diff (1.9 KB) - added by mfields 3 years ago.
Builds off previous diff. unset custom_cache_key before serialization.
13318.diff (736 bytes) - added by ryan 3 years ago.
cache_domain
13318.2.diff (1.4 KB) - added by mfields 3 years ago.
Adds aposthrophe to docs in 13318.2

Download all attachments as: .zip

Change History (32)

djudorange4 years ago

the patch

comment:1 nacin4 years ago

  • Milestone changed from 3.0 to Unassigned

A PHP comment very specifically says " $args can be whatever, only use the args defined in defaults to compute the key". This was changed in [7738].

comment:2 momo360modena4 years ago

Ok.

But why limit the bulding of the key at the $defaults variable ?
If a developer spends an additional parameter, it’s to be able to work on filters of the function.

Furthermore, generate the unique key by using $args cannot be blocking. In the worst, the request will be twice made.

But if the value of the args is different, the user thinks make 2 different things. Thus it is not logical to put in cache with the same key.

comment:3 nacin3 years ago

The point of the code is to only generate the cache key based on the variables that are included in the defaults. What's the use case for overloading $args with other keys?

comment:4 nacin3 years ago

  • Keywords reporter-feedback added

comment:5 ryan3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

comment:6 mfields3 years ago

  • Cc michael@… added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Hello,

I would like to reopen this ticket give a valid use case and hopefully provide an acceptable solution. I ran into this caching issue while adding functionality to my [taxonomy list shortcode]http://wordpress.org/extend/plugins/taxonomy-list-shortcode/ plugin. I thought that it would be a great idea to enable users to create a "glossary" of terms. I have one of these set up on my site: http://wordpress.mfields.org/glossary/

For this to work correctly I needed to hook into the 'terms_clauses' filter to append custom SQL that would only query for terms that have descriptions. I passed a custom argument to get_terms() and coded the 'terms_clauses' callback function to recognize this value.

This worked really well in cases where this was the only call to get_terms() in the script. If get_terms() was called elsewhere in the script with the same core-supported arguments defined but without my custom parameter, the custom query would be used because it is stored in the object cache.

I was able to piece together a hack to enable this to work that resets the cache key if my parameter was send:

function taxonomy_list_shortcode_reset_cache_key( $args ) {
	if ( isset( $args['taxonomy_list_has_description'] ) ) {
		wp_cache_set( 'last_changed', time() . '-description-only', 'terms' );
	}
	else {
		wp_cache_set( 'last_changed', time(), 'terms' );
	}

	return $args;
}
add_filter( 'get_terms_args', 'taxonomy_list_shortcode_reset_cache_key' );

While this is not a great solution, it was the only way that I could get the caching to work correctly.

The solution that I would like to offer would be to add a new default value to those recognized by get_terms(). This argument would have the key "custom" and it's value would be set to a string which could be used to construct a unique cache key for custom queries.

comment:7 mfields3 years ago

  • Keywords dev-feedback added

mfields3 years ago

Adds a recognized arg for "custom" which defaults to boolean false.

comment:8 nacin3 years ago

Perhaps it should be a flag that forces a comparison of all keys, not just defaults, and off by default. Needing to pass in an arg with a unique value sounds hacky.

mfields3 years ago

Less hacky approach as suggest by Nacin. Added docs too.

comment:9 mfields3 years ago

I believe that 13318.get-terms-custom-arg-2.diff is what you were driving at. Please correct me if I am wrong though. Wasn't really sure about variable names. Please suggest alternatives if these are not descriptive enough.

comment:10 Viper007Bond3 years ago

I've run into a similar issue before. I was passing a custom value to a function like this that a filter used to do something, but caching was getting in the way. I'm not sure if it was specifically this function, but it was the same type of issue and highly annoying. You're not alone in doing this kind of thing.

comment:11 mfields3 years ago

Glad to hear I'm not the only one :) It took me a while to figure out the hack posted above.

comment:12 nacin3 years ago

  • Milestone set to 3.2

I'm happy with the latest patch. 3.2 for consideration.

comment:13 nacin3 years ago

I think custom_cache_key might need to be unset so it doesn't get serialized in.

mfields3 years ago

Builds off previous diff. unset custom_cache_key before serialization.

comment:14 mfields3 years ago

Just wondering... Is the reason you pointed this out so that the cache stored in installations will not need to be regenerated when WordPress is updated?

comment:15 ryan3 years ago

The reason we use defaults instead of args is so that functions that call get_terms() and pass an args array that contains the arguments for both the calling function and for get_terms() will map to the same cache bucket for the same query. When all args are used to form the cache key, calls to get_terms() that result in the same query aren't usually cached to the same place. With custom_cache_key using all arguments, get_terms() calls with it set likely won't cache to the same place for the same query. Probably not a big deal, but it tempts me to go with mfields' original suggestion with something like 'cache_domain' => 'core' added to defaults and letting callers pass 'cache_domain' => 'custom-domain' if they need their own cache namespace.

comment:16 ryan3 years ago

  • Keywords needs-patch added; has-patch removed

We agreed on cache_domain during the bug scrub.

comment:17 scribu3 years ago

  • Keywords reporter-feedback dev-feedback removed

comment:18 scribu3 years ago

Taking a closer look, the $defaults array is always the same. Therefore, baking it into the cache key has no effect at all, so we can just skip it.

comment:19 ryan3 years ago

Perhaps I misunderstand what you mean, but compact( array_keys( $defaults ) ) is baking in the values extract()ed from $args. They are not always the same.

comment:20 scribu3 years ago

Ah, right. I'm the one who misunderstood then. :)

ryan3 years ago

cache_domain

comment:21 ryan3 years ago

13318.diff is the same approach as 13318.get-terms-custom-arg.diff but with cache_domain used as the key name as discussed in comment:15.

Last edited 3 years ago by ryan (previous) (diff)

comment:22 ryan3 years ago

  • Keywords has-patch added; needs-patch removed

comment:23 mfields3 years ago

13318.2.diff builds on Ryan'spatch adding inline documentation for the new argument. This was a bit harder to document than I thought. Suggestions on improvements are totally welcome.

comment:24 Viper007Bond3 years ago

Makes sense, although you're missing an apostrophe: this function's filters

mfields3 years ago

Adds aposthrophe to docs in 13318.2

comment:25 mfields3 years ago

Viper007Bond: Good catch. Updated 13318.2.diff to add the punctuation.

comment:26 ryan3 years ago

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

In [18128]:

Add cache_domain argument to get_terms() to allow caching to a unique set of cache buckets. Useful when taxonomy queries have been modified via filters and need their own cache space. Props mfields. fixes #13318

Note: See TracTickets for help on using tickets.