WordPress.org

Make WordPress Core

Opened 8 years ago

Last modified 9 months ago

#24386 new enhancement

Make _pad_term_counts work for non-post type objects and attachments

Reported by: TomAuger Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

If you register a hierarchical taxonomy against a non-post object (such as users), or attachments that aren't associated with another post, _pad_term_counts does bubkus, due to the INNER JOIN on $wpdb->posts, and the check for 'publish' status (which is relevant for attachment post types).

I'm suggesting an alternative approach would be to use $term->count, and eschew going back to the database altogether.

Attachments (1)

24386.diff (13.1 KB) - added by david.binda 9 months ago.

Download all attachments as: .zip

Change History (7)

#1 @ocean90
8 years ago

Related: #22558

#3 @wonderboymusic
8 years ago

  • Keywords needs-patch added

#4 @boonebgorges
7 years ago

  • Keywords needs-unit-tests added

I'm suggesting an alternative approach would be to use $term->count, and eschew going back to the database altogether.

I agree that it's bad to have this function so closely tied to posts. But simply incrementing 'count' seems risky, as there'd be no checks to make sure these numbers stayed in sync.

Some unit tests for this functionality would be hugely helpful, as it'd give us a better sense of when these counts ought to be changing.

#5 @boonebgorges
7 years ago

  • Milestone changed from Awaiting Review to Future Release

Circling back around to this, I think I agree that we should be using $term->count, and not doing a separate database query. I'm not sure why the query was ever necessary; see [6159], [6087], [4707] for some relevant history. The 'post_type' information does not seem important, since only the proper object_types should be reflected in the 'count' field; likewise for the 'post_status' check.

The key will be to write some unit tests that verify that the 'count' field is correctly recalculated on 'transition_post_status'. This should already be happening at _update_term_count_on_transition_post_status(), and the term count callback functions (_update_post_term_count() and _update_generic_term_count()) should already be accounting for post_type/post_status stuff, but this needs verification via tests.

#6 @david.binda
9 months ago

With #40351 being worked on, I wonder if it's a good time to revisit this ticket.

I think I agree that we should be using $term->count, and not doing a separate database query. I'm not sure why the query was ever necessary;

Reviewing the revisions mentioned above, as well as current usage of the pad_counts argument of the get_terms function, I don't believe that the database query is meant for accuracy of the numbers displayed.

The _pad_term_counts function is only called in case the pad_counts arg is explicitly set to true (it default to false). Disregarding the current implementation, which recalculates the counts from scratch, I'd say the _pad_term_counts function is only really meant for count padding in hierarchical display, so the parent term count contains numbers for all it's children, not for ensuring the accuracy of the numbers, especially as there is no such accuracy check performed when the pad_counts arg is set to false (default behaviour).

The key will be to write some unit tests that verify that the 'count' field is correctly recalculated on 'transition_post_status'.

Those seem to be worked on in terms of r49141 (yet currently reverted).

From my point of view, it makes sense to move towards a solution which takes advantage of the $term->count value for calculating the padding of ancestor terms, as it would significantly increase the performance of the function in terms of reducing SQL queries as well as lowering the memory usage.

I have attempted to implement $term->count approach by modifying the Walker class to start with some solid and tested implementation ( see the attached diff ).

@david.binda
9 months ago

Note: See TracTickets for help on using tickets.