[wp-trac] [WordPress Trac] #21760: get_term_by() calls are not cached

[WordPress Trac]

#21760: get_term_by() calls are not cached
------------------------------+-----------------------------
 Reporter:  wonderboymusic    |       Owner:  wonderboymusic
     Type:  enhancement       |      Status:  assigned
 Priority:  normal            |   Milestone:  4.0
Component:  Taxonomy          |     Version:  2.3
 Severity:  normal            |  Resolution:
 Keywords:  has-patch commit  |     Focuses:
------------------------------+-----------------------------

Comment (by tollmanz):

 Nice work! I love the idea of `wp_get_last_changed()`. An incrementor API
 like this would be most excellent in core.

 A couple thoughts on the patch:

 * Line 980 of **src/wp-includes/taxonomy.php** should use `$term->term_id`
 as the key, not the `$term` object.
 * `$bucket` is not a term used in core (other than fleetingly in a few
 comments). I think for clarity and consistency, we should stick with the
 `$group` nomenclature.
 * The keys for the term names will prove problematic. Whitespace is not
 allowed in Memcached keys. Additionally, these keys can only be a grand
 total of 250 characters in Memcached (and I'm assuming there are other
 limits in other object caches). These should be hashed or sanitized in
 someway before being used as keys.
 * Playing devil's advocate, if a term is not found in `get_term_by()`,
 should that result still be cached to avoid the redundant look up in the
 future? I think that would be nice, but complicates checking for the "non-
 existent" values, as well as the invalidation of that cache.
 * I don't think `update_term_cache()` will work. 1) Using `wp_cache_add()`
 will fail to save new data as it is already set. A quick and dirty fix for
 this is to use `wp_cache_set()`. It'll guarantee new data; however, there
 is a slight performance penalty. 2) Since we are using incrementors, we
 should...well...use them. We should generate a new incrementor and save
 the new values with the new incrementor. This causes a cache miss on the
 old incrementor and saves the new one. This could be accomplished by
 adding a `$force` parameter to `wp_get_last_changed()`. If set to `true`,
 it would not pull the incrementor from cache and instead regenerate it and
 save over the old one. Alternatively, it could just erase the old one.

 Finally, as excited as I am about the incrementor idea, I do have some
 concerns. Incrementors work great with caches that use an LRU eviction
 policy (e.g., Memcached). You can fairly carelessly add incrementors to
 cause cache misses/invalidations. This works well because the old
 incrementors are no longer accessed and eventually fall off the slab. With
 some caching systems, the eviction policy is a bit problematic for an
 incrementor system (e.g., APC fills the cache until full, them dumps
 everything). We need to be really careful about how we do this to make
 sure it will consistently work across object caches. Ideally, when we
 change the incrementor, we delete the old one or overwrite it.

 I think we need to be very careful as we implement this. An incrementor
 system can be extremely powerful, yet also really dangerous. Perhaps we
 should start a separate ticket to explore this? If we were to implement
 this in core, it would be great to have a more robust system for handling
 this with methods for setting, getting, and invalidating incrementors.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/21760#comment:44>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform