Opened 11 years ago
Last modified 4 years 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)
Change History (7)
#4
@
10 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
@
10 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
@
4 years 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 ).
Related: #22558