[wp-trac] [WordPress Trac] #37198: Use `WP_Term_Query` for `wp_get_object_terms()`

[WordPress Trac]

#37198: Use `WP_Term_Query` for `wp_get_object_terms()`
-----------------------------------+------------------
 Reporter:  flixos90               |       Owner:
     Type:  enhancement            |      Status:  new
 Priority:  normal                 |   Milestone:  4.7
Component:  Taxonomy               |     Version:  4.6
 Severity:  normal                 |  Resolution:
 Keywords:  has-patch 2nd-opinion  |     Focuses:
-----------------------------------+------------------
Changes (by boonebgorges):

 * keywords:  early has-patch => has-patch 2nd-opinion
 * milestone:  Future Release => 4.7


Comment:

 @flixos90 Thank you for the hard work you've done on the first pass, and
 sorry for the time it took to get around to reviewing it. It's a hard one
 :)

 Your patch is extremely conservative, in the sense that (a) it guarantees
 100% backward compatibility, and (b) it attempts not to touch any existing
 code beyond what must be touched to refactor `wp_get_object_terms()`. I
 like this as a general approach :) But in this case, I think that we can
 simplify our strategy by looking a bit deeper at the back compat issues.

 [attachment:37198.2.diff] is a somewhat simplified version of your patch.
 (It's also sloppy - I left out some documentation and default params. This
 will be cleaned up.) Here are the important differences:

 - ` // Verification of term hierarchy must be skipped for
 wp_get_object_terms().` If I'm reading it correctly, this block in
 [attachment:37198.diff] was meant to address
 `Test_Term_WpGetObjectTerms::test_parent()`. These tests fail without the
 block. The reason is that the tests were written incorrectly: they define
 hierarchical terms, in a taxonomy that is non-hierarchical. By changing
 the tests, we can avoid the additional logic. This is technically a
 compatibility break. Currently, if you manually create hierarchical terms
 in a non-hierarchical taxonomy, and you call `wp_get_object_terms()` with
 'parent', only the object-terms with the correct 'parent' will be
 returned. With my patch, doing this same thing will result in an empty
 array being returned. IMO, passing 'parent' when the taxonomy is non-
 hierarchical is not meaningful, and it's OK to break it - but I would be
 interested to hear what you think.

 - When building the `SELECT` clause, I default to selecting `t.*, tt.*`
 whenever possible. The performance differences are negligible, and keeping
 a smaller number of variations in the SQL query means better cache
 coverage. Note that this part of the patch will need refreshing when the
 main `SELECT` clause in `WP_Term_Query` grabs only the `term_id` (or
 `term_taxonomy_id`) column. See #37189, #34239.

 - I removed the `post_process_terms()` logic. The sanitization you mention
 was introduced in [26010] to ensure that integer values are, in fact,
 integers. But we now have `WP_Term` for this purpose. So, instead, my
 patch sends results through `get_term()` when `fields=all_with_object_id`,
 and also adds some integer-casting when `fields=ids` and some other
 numeric values. I think this logic can probably be simplified even further
 with #34239 etc, but for now I think it's clearer to ensure that `WP_Term`
 objects are returned.

 - You had a special clause for 'orderby' in `wp_get_object_terms()` to
 'id'. I think this is related to the fact that some of our unit tests
 passed `orderby=t.term_id` to `wp_get_object_terms()`. (You also have some
 logic in `parse_orderby()` for this.) But I'm fairly sure that these tests
 are incorrect: `t.term_id` has never been officially supported as a value
 for 'orderby'. It worked in some cases because, since it didn't match a
 whitelisted 'orderby' value, it fell through to the default - which
 happened to be `term_id`. However, I don't think this is something that we
 must continue to support. I could be convinced otherwise if we found a
 bunch of uses of `orderby=t.term_id` in the wild, but even then I'd
 strongly consider the break. My patch fixes the tests, instead.

 So, my approach is a bit more cavalier, but results in a considerably
 smaller patch. What do you think, @flixos90 ?

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