[wp-trac] [WordPress Trac] #37038: WP_Tax_Query needs caching

[WordPress Trac]

#37038: WP_Tax_Query needs caching
--------------------------+--------------------------
 Reporter:  spacedmonkey  |       Owner:
     Type:  enhancement   |      Status:  new
 Priority:  normal        |   Milestone:  4.9
Component:  Query         |     Version:  3.1
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:  performance
--------------------------+--------------------------
Changes (by boonebgorges):

 * keywords:  needs-unit-tests needs-refresh has-patch => has-patch
 * milestone:  Future Release => 4.9


Comment:

 Thanks for the updated patch, @spacedmonkey. I think we are close on this
 one. [attachment:37038.6.patch] makes the following changes to 5.patch:

 - I've removed the `$resulting_fields` -> `$args['fields']` switch
 statement. This means that all calls to `WP_Term_Query` will fetch
 `fields=all`. This seems better to me for a couple reasons. First, it
 primes the cache for future use (as you note in [comment:4 your comment
 above]). Second, it means we can simply `wp_list_pluck()` at the end of
 the method in all cases. Third, it helps us move toward a future where the
 'fields' parameter in `WP_Term_Query` doesn't result in fragmented caches.
 - On 576 of [attachment:37038.5.patch], you added an `array_filter()`. It
 looks like this was meant to break some broken tests that resulted from
 some edge cases related to empty values being passed to the `terms`
 parameter of `tax_query` clauses. Your fix worked, but it felt opaque to
 me, so I put a more explicit fix into `transform_query()` (bailing if
 `'terms'` is empty), which is where the problem really lives anyway.
 - I made a few changes to tests that expected to get string IDs instead of
 true integers - eg `'123'` instead of `123`. This is technically a
 compatibility break, which we should take a note of and try to document in
 a dev note for 4.9, but it's highly unlikely that anyone is doing a strict
 string-check on the result of a `transform_query()` transformation.
 - 'suppress_filters' is a parameter for `get_terms()`, but since we're
 using `WP_Term_Query` directly, we don't need it.

 I've reviewed the existing `transform_query()` tests and they look fairly
 comprehensive, so I'm not sure that we need new ones.

 @spacedmonkey Mind having a look through the updated patch to see if
 anything jumps out at you?

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