[wp-trac] [WordPress Trac] #37189: In wp_term_query on cache ids

[WordPress Trac]

#37189: In wp_term_query on cache ids
--------------------------+--------------------------
 Reporter:  spacedmonkey  |       Owner:
     Type:  enhancement   |      Status:  new
 Priority:  normal        |   Milestone:  4.9
Component:  Taxonomy      |     Version:  4.6
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:  performance
--------------------------+--------------------------

Comment (by boonebgorges):

 Thanks to @spacedmonkey and @flixos90 for the thorough feedback. I've gone
 ahead with the `clean_taxonomy_cache()` and `_split_shared_term()` fixes,
 since they're not dependent on the rest of this ticket.

 Regarding cache keys: Yes, I've just reread a comment from previous-me and
 I think there are non-stylistic reasons why we need to hash the args:
 https://core.trac.wordpress.org/ticket/21267#comment:17 `$taxonomies` is
 part of the `$args` array, so that part is redundant. I will make the
 change.

 > The patch remove the cache expiry time of DAY_IN_SECONDS on the term
 cache. I have never understood why this was put in, as if cache
 invalidation is correct, then it should never be needed.

 Good catch. The one-day timeout was introduced in [15583]; see #11431.
 Since this time, we have had a clearer position on invalidation: it's
 generally the job of the cache engine (and its administrator) to set a
 policy for evacuating old content. So I think it's fine to remove the
 restriction.

 > The all_with_object_id value is (in addition of count) the only value
 for the fields key that requires a different information that simply an
 array of IDs being in the cache.

 Yes, this is a good point. It will take some non-trivial refactoring to
 make this work right. It also appears that we don't have the right kinds
 of tests to confirm the cache behavior of this sort of query (surprising,
 since it was added recently). I'll work on this.

 > While I like the idea of not running the get_term() queries when only
 ids are queried a lot, I'm a bit wary this could cause unexpected issues,
 especially surrounding the hacky logic of manually creating stdClass
 objects with the term_id property set.

 Yes, this part of the patch did not feel great to write. I'm fairly sure
 that there is no current possibility for bugs due to the `stdClass` hack
 (as you note, none of the relevant `if` clauses later in the function are
 tripped). But future WordPressnauts may be tripped up by this bit of
 weirdness. There may be a way to accomplish much the same thing in a way
 that feels less unpleasant, maybe by explicitly checking for `$_fields =
 'ids'` on some of those latter `if` clauses, and/or having a separate `if`
 block for formatting the return value when we've only got an array of
 integers (to avoid `stdClass` juggling). I'll think about it.

 I'll try to wrestle all of this into a new patch in the upcoming days.

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