Comments: Add wp_update_comment_counts() to reconcile stored counts - #58
Open
adamsilverstein wants to merge 7 commits into
Open
Conversation
wp_update_comment_count_now() only refreshes a post's stored comment_count when that post's comments change, so an existing count can become stale after the set of excluded comment types changes (for example when a plugin registers a type that opts out of default listings via default_excluded_comment_types). Registration runs on every request and is not a state transition, so there is no safe automatic trigger; the established core pattern for this is an explicit recount, like flush_rewrite_rules() for rewrite rules. Add a bulk recount helper that recomputes one or more posts' counts through wp_update_comment_count_now(), so it honors the same exclusion filter. A plugin that changes the excluded set calls it once, typically on activation. See #35214, #65537.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…re/65537-update-comment-counts
- Bump the comment 'last_changed' cache key: comment query results are salted only by that key, so on a persistent object cache the activation-time flow (register the exclusion filter, then recount) would otherwise keep serving results cached under the previous excluded set. - Include posts with a nonzero stored count but no remaining comment rows in the null path: SELECT DISTINCT on the comments table cannot see a post whose rows were all deleted while its stored count is stale. - Iterate the null path in keyset batches of 1000 instead of materializing every post ID in memory. - Skip negative IDs instead of silently recounting their absolute value, and document the per-post side effects (edit_post hooks fire for cache purgers and indexers) alongside the cache-invalidation behavior. - Tests: null-path return value, invalid/negative/nonexistent IDs, stale-count reset for commentless posts on both paths, and the last_changed bump.
…re/65537-update-comment-counts
The keyset loop was correct but its query was not batched in any useful sense. MySQL cannot push an outer ORDER BY ... LIMIT into a parenthesized UNION arm, so every iteration materialized all remaining rows of both arms into a temp table before taking its thousand - and since comment_count is unindexed, the posts arm scanned the whole remaining table each time. A recount of N commented posts cost roughly N squared over the batch size, which defeats the batching that is the whole point of the null path. Give each arm its own ORDER BY and LIMIT. Keyset semantics are unchanged: the first N rows of the union of two ascending sets are always among the first N of each, so the batch is the same batch, read with a bounded walk of each index. The batch size becomes filterable along the way, which also makes it testable across a boundary - including a post that has both comments and a stale count, so it turns up in both arms. See #65537.
The comment last_changed key was bumped first thing, before the post IDs were even validated, so wp_update_comment_counts( array() ) or a list of nothing but invalid IDs invalidated every cached comment query on the site while recounting nothing at all. Bump it once there is at least one post to visit. The existing cache test relied on that no-op bump, so it now recounts a real post, and the no-op cases get their own test asserting the key is left alone. See #65537.
The docblock recommended calling this from a plugin activation routine while also warning it can be expensive, which is contradictory advice: activation runs in a normal web request, and a full recount on a large site will hit max_execution_time partway through. Point at wp_schedule_single_event() or WP-CLI for that case, and say plainly that stopping partway is safe - posts visited before the stop are correct and the operation is idempotent. Three more things the function does that a caller could only learn by reading it: counts are written immediately rather than joining the wp_defer_comment_counting() queue, post IDs that do not exist are skipped and left out of the return value, and there is no capability check, so anything exposing this to a request has to add its own. See #65537.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the #12310 counter fix, addressing the second half of pfefferle's review on Trac #35214 comment:52: keeping a post's stored
comment_countconsistent when the set of excluded comment types changes.The gap
wp_update_comment_count_now()(made filter-aware in WordPress#12310) only refreshes a post's storedcomment_countwhen that post's comments change. So a count written before a type joined the excluded set stays stale until the post sees activity again. Example: a site runs withreviewcomments counting normally, then installs a plugin that excludesreviewviadefault_excluded_comment_types- existing posts keep their inflated count.Why not auto-recount on registration
register_comment_type()runs on every request duringinit; it is not a persisted state transition, and the exclusion set is driven by thedefault_excluded_comment_typesfilter, not by register/unregister. There is no clean event to hook. Investigated alternatives and rejected them:unregister_comment_type- wrong event (exclusion changes come from the filter, not unregister) and per-request.UPDATEduring aGET(breaks read replicas / page caches;get_comments_number()is deliberately read-only) and relies on diffing a per-request filter output on the hot path.The closest core analog confirms the right pattern: taxonomy term counts are recalculated only on data events, never on taxonomy registration. Rewrite rules use the same model - core exposes
flush_rewrite_rules()and documents "call it on activation" rather than auto-detecting.This PR
wp_update_comment_counts( $post_ids = null )- a bulk recount helper that recomputes one or more posts' counts through the now filter-awarewp_update_comment_count_now(), so it honors the same exclusion set by construction (no new SQL). A plugin that changes the excluded set calls it once, typically from its activation hook; it also gives a future WP-CLI/admin maintenance tool a single correct entry point.null(default) recalculates every post that has at least one comment.Naming follows the core
wp_update_*_counts()bulk family (wp_update_user_counts(),wp_update_network_counts()); singularwp_update_comment_count()already exists for a single post.Tests
New
tests/phpunit/tests/comment/wpUpdateCommentCounts.php: empty input returns 0; targeted IDs only touch those posts; duplicate IDs are deduped;nullrecalculates all posts with comments; and the headline case - a newly-excludedreviewtype drops a previously stored count to 0.--group commenttests pass (557 + 5 new).Review updates
wp_update_comment_counts()now bumps the commentlast_changedcache key. Comment query caches are salted only by that key, so the activation-time register-filter-then-recount flow is now fully self-consistent on persistent object caches.nullpath additionally visits posts with a nonzero stored count but no remaining comment rows - aSELECT DISTINCTon the comments table cannot see those - and iterates in keyset batches of 1000 instead of materializing every post ID.absint()-coerced into valid ones.edit_posthooks fire per recount, so cache purgers and indexers run once per post.last_changedbump.Stacked on WordPress#12310 (
feature/65537-excluded-comment-types-filter); retarget totrunkonce that lands.Following a second review pass over the stack:
LIMIT. MySQL cannot push an outerORDER BY ... LIMITinto parenthesized union arms, so on a large site each iteration materialized both full result sets before trimming to a batch. Each arm now carries its own orderedLIMIT, which is semantics-preserving: the first N rows of the union of two ascending sets are always among the first N of each.last_changedbump ran before the early return, sowp_update_comment_counts( array() )invalidated every cached comment query on the site while recalculating nothing. It now happens only when there is something to recount.wp_update_comment_counts_batch_size, floored at 1, so a site with an unusual comment distribution can tune it and the tests can exercise the batch boundary without creating thousands of posts.Testing
(The skipped test is WordPress#12310's registry-integration case, which guards on
get_comment_types()existing. This branch is stacked on WordPress#12310 but not on the registration API in #12311, so it correctly skips here and runs once the two meet.)PHPCS reports no new warnings on the changed files and PHPStan is clean.
AI Use
Code and description both written with 🤖 Claude Code. I will review and test.