[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 flixos90):

 Great work on this so far, I agree it generally makes sense to follow the
 other query classes and query IDs only to make the caches applicable more
 generally.

 I jumped into the code for a while and reviewed it. Here are my thoughts:

 * Even though it looks a bit less elegant, I prefer generating the cache
 key in the way that @spacedmonkey did in [attachment:37189.2.patch]. His
 idea has parity with other query classes and I don't see any harm in
 manually removing the `fields` key from the arguments unless its value is
 `count` or `all_with_object_id` (see below).
 * 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. Given my above point, I think
 it would make sense to add this field to the condition in
 [attachment:37189.2.patch], in order to leave it in the key generator
 array under these circumstances. Some logic would need to be added then in
 [attachment:37189.3.patch] in the same location where the special `count`
 cache is handled, in order to set an array of objects in the cache where
 each object contains `term_id` and `object_id` properties. This
 information would be sufficient to create the objects. In case of a cache
 hit, the `term_id` field could be used to create an array of term objects
 via calls to `get_term()` and then the `object_id` simply needs to be set
 for each object.
 * In [attachment:37189.3.patch] the check for `! empty(
 $args['object_ids'] )` is missing: The `tr.object_id` field should only be
 queried if actual object IDs have been passed, similar as before.
 * 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. Looking for any
 unexpected calls to other term properties (which would not exist in that
 case), I wasn't able to detect any issue since all such cases should be
 prevented by respective if-clauses. However, since this part is rather
 complex, let's pay particular attention to this change. We need thorough
 unit tests for this especially.
 * +1 on fixing the `_split_shared_term()` bug and introducing a
 `clean_taxonomy_cache()` function. The action name, as noticed by
 @spacedmonkey above, should be `clean_taxonomy_cache`, I assume this was
 just a typo.

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