Bug caching get_terms

[djudorange]

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.

[djudorange]

the patch

[nacin]

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

[momo360modena]

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.

[nacin]

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?

[mfields]

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.

[mfields]

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

[nacin]

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.

[mfields]

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

[mfields]

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.

[Viper007Bond]

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.

[mfields]

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

[nacin]

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

[nacin]

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

[mfields]

Builds off previous diff. unset custom_cache_key before serialization.

[mfields]

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?

[ryan]

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.

[ryan]

We agreed on cache_domain during the bug scrub.

[scribu]

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.

[ryan]

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.

[scribu]

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

[ryan]

cache_domain

[ryan]

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.

[mfields]

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.

[Viper007Bond]

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

[mfields]

Adds aposthrophe to docs in 13318.2

[mfields]

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

[ryan]

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