[wp-trac] [WordPress Trac] #38280: Make term count for a specific object type available

[WordPress Trac]

#38280: Make term count for a specific object type available
--------------------------------------+------------------------------
 Reporter:  desrosj                   |       Owner:  (none)
     Type:  enhancement               |      Status:  new
 Priority:  normal                    |   Milestone:  Awaiting Review
Component:  Taxonomy                  |     Version:  4.7
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:
--------------------------------------+------------------------------

Comment (by desrosj):

 Thanks for the patch refresh, @ivankristianto!

 Looks like the changes in [attachment:38280.8.diff] are breaking the unit
 tests. After looking into it further, I think those changes should be
 reverted (which I have done in [attachment:38280.9.diff]). While the
 `term_id` could be different than `term_taxonomy_id`, the function's
 inline documentation explicitly specifies `List of Term taxonomy IDs.`. I
 also looked at where `_update_post_term_count()` is called in core and the
 only function that calls it is `wp_update_term_count_now()`. This
 function's documentation also specifies `The term_taxonomy_id of terms to
 update.`, so I think we can avoid that additional query.

 A term ID check ''may be'' needed to ensure the correct term ID for the
 meta function calls, but I need to look into this further. @boonebgorges
 does anything stick out to you there?

 Also in [attachment:38280.9.diff]:
 - Change version number on new functions to `5.0.0`.
 - Added a `@since` tag to `_update_post_term_count()` noting the new meta
 information.
 - If a `WP_Error` is returned by `get_term()` return from
 `wp_get_term_count_for_object_type()`. Previously, it was just assumed a
 term would always be returned. `null` can also be returned for
 "miscellaneous failures". I was looking to better understand when `null`
 would be returned so I could write tests to catch it before adding a
 return statement for this condition.
 - Removed the `else` statement at the bottom of
 `_update_post_term_count()`. The meta entries deleted in this `else` are
 already deleted earlier on in the function.
 - Moved tests for `wp_get_term_count_for_object_type()` into a separate
 test class.

 Replying to [comment:10 boonebgorges]:
 > Can you explain this logic to me?
 >
 > {{{
 >       * If the object type has been counted, and other counts exist in
 meta, we can
 >       * assume this term has 0 relationships for this object type.
 > }}}
 >
 > Why is it necessary to check for *other* counts? Shouldn't it be enough
 for the type to be listed in `_wp_counted_object_types`? If there's a
 scenario where these values might get out of whack, we should (a) have a
 test that describes it, and/or (b) defend against that scenario :)

 I think this was something I missed when previously when adding the
 `_wp_counted_object_types` meta cache. I have removed it.

 > This reorganization shows that there are some cases in which
 wp_get_term_count_for_object_type() doesn't return a value > at all.

 Looking at this further, it seems the only time we would not return a
 value would be if `wp_update_term_count_now()` returned a "falsey" value.
 But, that function always returns `true`. Wondering if this could ever
 cause an endless loop in the last conditional of the function (if the meta
 does not successfully get stored, it may call itself recursively).

 I think this is in a great spot now pending the items above. I would like
 to cut the test methods into smaller ones (and maybe use some fixtures) as
 well. I couldn't find any tests for `wp_update_term_count_now()` or
 `_update_post_term_count()` either. If we don't add them here, a new
 ticket may be good for tracking.

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