Repository navigation
Comments: Add filter for comment types excluded from queries by default - #12310
adamsilverstein wants to merge 47 commits into
Conversation
WP_Comment_Query hard-codes the exclusion of the 'note' comment type (introduced in 6.9) with no extension point. Plugins that add their own "private" comment types must instead rewrite query SQL through comments_clauses in every context and re-verify it on each release. Introduce a default_excluded_comment_types filter so extenders can contribute additional comment types to the default-excluded set, generalizing the existing 'note' handling. The default value of array( 'note' ) preserves current behavior exactly. See #65537.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
|
Hi there! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…cess control. The filter docblock described excluded types as "private", which could be read as a security boundary. The exclusion only governs default visibility; excluded types remain retrievable via explicit 'type' or 'all' requests, so document that callers must enforce capability checks wherever comment data is displayed or exposed.
wp_update_comment_count_now() recalculated a post's stored comment_count with a hard-coded 'AND comment_type != note', bypassing the default_excluded_comment_types filter that hides those types from WP_Comment_Query. A type that opted out of listings still inflated comment_count and therefore get_comments_number(). Apply the same filtered exclusion set to the counter so the listings and the stored count stay consistent, removing the need for plugins to also re-implement the count via pre_wp_update_comment_count_now. See #35214, #65537.
get_pending_comments_num() recomputed the admin pending-comments count with a hard-coded 'AND comment_type != note', the same gap fixed in wp_update_comment_count_now(): a type that opts out of default listings via default_excluded_comment_types still inflated the pending bubble shown next to each post in the admin list tables. Derive the excluded set from the same filter so the pending count stays consistent with the listings and the stored comment count. See #35214, #65537.
The default-excluded set plus filter application was triplicated across
WP_Comment_Query, wp_update_comment_count_now(), and
get_pending_comments_num(), each with subtly different normalization.
Centralize it in one accessor that all three call, which is also the
seam where a future comment type registry can derive the default from
internal types.
The accessor additionally strips the special type tokens understood by
WP_Comment_Query ('all', 'comment', 'comments', 'pings'): previously a
filter returning the 'pings' alias excluded nothing from the count
functions but zeroed out an explicit type => 'pingback' query, because
the explicit-request guard compares raw tokens while the query expansion
resolves aliases.
Also document the registration-timing contract (register the filter
unconditionally rather than toggling per call - the query cache key does
not include filter output), widen the null-context description, hoist
the invariant 'all' check out of the per-type loop, add the missing
@SInCE entry to wp_update_comment_count_now(), and make the
no-duplication test capture the WHERE clause via comments_clauses
instead of asserting against $wpdb->last_query.
…pes.
When default_excluded_comment_types excludes a literal ping type (for
example 'pingback'), a query requesting the 'pings' alias built
`comment_type IN ('pingback','trackback') AND comment_type NOT IN
('pingback')`, silently dropping the explicitly requested pingbacks. The
guard compared excluded literals against the raw request tokens, so an
alias never matched the literal it stands for.
Expand the request's special tokens ('comment', 'comments', 'pings') to
their literal comment_type values before deciding what to exclude, so a
type requested via an alias is honored. This also drops the redundant
NOT IN membership check, since the list is deduplicated with
array_unique() before the clause is built.
The normalization in wp_get_default_excluded_comment_types() ran the filtered list through array_filter() with no callback, which drops the string '0' along with empty strings. A comment type literally named '0' could therefore never be excluded through the filter. Filter on strlen() instead, so only empty strings are removed and '0' survives as a valid literal type.
The default_excluded_comment_types docblock said the filter keeps types out of listings, counts, "or feeds". Comment feeds are served by WP_Query's raw SQL, which does not consult this filter, so the promise does not hold. Describe only the surfaces the filter actually covers.
The list table hardcoded `type__not_in => array( 'note' )` for its row query while the status count bubbles (via wp_count_comments()) now honor default_excluded_comment_types. A plugin that removed 'note' from the filter would therefore see the count include notes while the rows still hid them, so "All (N)" disagreed with the visible list. Source the row query's exclusions from wp_get_default_excluded_comment_types() so the list and its counts stay in agreement.
array_filter() with 'strlen' was doing the right thing at runtime - dropping '' while keeping a type literally named '0' - but strlen() returns an int, not a bool, so PHPStan reports the callback as the wrong type and the static analysis job fails. Say what the filter means instead. See #65537.
…ath. get_page_of_comment() counted older comments with 'type' => 'all', which is the one value that makes WP_Comment_Query skip its default exclusions, while comments_template() renders the same post with those exclusions applied. Every excluded-type comment older than the target therefore inflated the count, so a '#comment-' permalink could point at a page the visitor never sees. Two notes ahead of a comment on a two-per-page post were enough to send its permalink to page 2 while the comment renders on page 1. Default the argument to '' instead, which is WP_Comment_Query's own default and so counts exactly what the comment list shows. Callers wanting the old behavior can still pass 'all' explicitly. get_comment_link() forwards its arguments here and carried the same default, so it moves too, otherwise the fix would never reach the permalinks that need it. This predates the exclusion filter - it has been reachable since 6.9 gave core the note type - but the filter generalizes it to any private type a plugin registers. See #65537.
The branch was written while trunk was 7.1-alpha; trunk is now 7.2-alpha, so the eleven @SInCE tags this branch adds pointed at a release that will never contain them. Docblock-only change. See #65537.
| // Unless all types are requested, exclude each default-excluded type | ||
| // that the query does not explicitly request. The special type tokens | ||
| // in the request ('comment', 'comments', 'pings') are first expanded to | ||
| // the literal comment_type values they represent, so a type requested | ||
| // via an alias (for example 'pings' for 'pingback' and 'trackback') is | ||
| // still treated as explicitly requested and is not excluded. |
There was a problem hiding this comment.
| // Unless all types are requested, exclude each default-excluded type | |
| // that the query does not explicitly request. The special type tokens | |
| // in the request ('comment', 'comments', 'pings') are first expanded to | |
| // the literal comment_type values they represent, so a type requested | |
| // via an alias (for example 'pings' for 'pingback' and 'trackback') is | |
| // still treated as explicitly requested and is not excluded. | |
| /* | |
| * Unless all types are requested, exclude each default-excluded type | |
| * that the query does not explicitly request. The special type tokens | |
| * in the request ('comment', 'comments', 'pings') are first expanded to | |
| * the literal comment_type values they represent, so a type requested | |
| * via an alias (for example 'pings' for 'pingback' and 'trackback') is | |
| * still treated as explicitly requested and is not excluded. | |
| */ |
There was a problem hiding this comment.
Applied in 280fcd6. I swept the rest of the diff for the same thing and converted two more: the switch in WP_Comments_List_Table::prepare_items() and the 'pings' alias note in the query tests. The remaining two-line runs are a prose line followed by a phpcs:ignore directive, which has to stay //, so I left those alone.
The WordPress inline documentation standards call for the /* */ block format once a comment runs past a single line.
The requested type is subtracted from the default-excluded set with array_diff(), which casts its arguments to strings. A request such as comment_type[]=review raised an "Array to string conversion" warning and could skip the exclusion it named.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DxbbMKz3sa741SjHuE1ZLS
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've added a few notes inline.
I'm a little worried about the subtly introduced between "all" and "" for some of hte comment APIs.
It seems risky for themes that have copied over the default value to the functions call in the past.
| } | ||
|
|
||
| $placeholders = implode( ', ', array_fill( 0, count( $excluded_types ), '%s' ) ); | ||
| $clause = sprintf( ' AND %s NOT IN ( %s )', $column, $placeholders ); |
There was a problem hiding this comment.
Column name is an identifier:
| $clause = sprintf( ' AND %s NOT IN ( %s )', $column, $placeholders ); | |
| $clause = sprintf( ' AND %i NOT IN ( %s )', $column, $placeholders ); |
There was a problem hiding this comment.
Good call, switched in 94dc58f.
Claude caught a snag with the straight swap:
%iquotes the whole value as one identifier, so the feed call sites passing"{$wpdb->comments}.comment_type"would have come out as`wp_comments.comment_type`- a single column name that doesn't exist. The helper now splits on the dot and uses one%iper part, giving`wp_comments`.`comment_type`. There's a new test covering both the plain and the qualified forms.
| remove_filter( 'default_excluded_comment_types', '__return_false' ); | ||
|
|
There was a problem hiding this comment.
Removed by base test class.
| remove_filter( 'default_excluded_comment_types', '__return_false' ); |
There was a problem hiding this comment.
Dropped these here and in the other test files in 14dc7d6.
|
|
||
| $counts = get_pending_comments_num( array( $post_a, $post_b ) ); | ||
|
|
||
| $this->assertSame( |
There was a problem hiding this comment.
| $this->assertSame( | |
| $this->assertSameSets( |
🔢 This applies in a few places so I won't repeat myself.
There was a problem hiding this comment.
Switched in 14dc7d6. This one uses assertSameSetsWithIndex(), since plain assertSameSets() sorts away the post ID keys the test is checking.
| $filter_args = array(); | ||
|
|
||
| add_filter( | ||
| 'default_excluded_comment_types', | ||
| static function ( $types, $query ) use ( &$filter_args ) { | ||
| $filter_args = array( $types, $query ); | ||
| return $types; | ||
| }, | ||
| 10, | ||
| 2 | ||
| ); |
There was a problem hiding this comment.
This could use MockAction
| ) | ||
| ); | ||
|
|
||
| $this->assertSame( array( $comments['pingback'] ), array_map( 'intval', $found ) ); |
There was a problem hiding this comment.
Tests with multiple assertions will need to contain a unique $message parameter for each to aid debugging for future contributors.
There was a problem hiding this comment.
Added messages across the new tests in 14dc7d6.
| $runs = 0; | ||
|
|
||
| add_filter( | ||
| 'default_excluded_comment_types', | ||
| static function ( array $types ) use ( &$runs ): array { | ||
| ++$runs; | ||
| return $types; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Could use MockAction too.
| * @return string Prepared ` AND <column> NOT IN (...)` condition, or an empty string when | ||
| * no comment types are excluded. | ||
| */ | ||
| function _wp_get_excluded_comment_types_clause( $column = 'comment_type' ) { |
There was a problem hiding this comment.
For a while, WordPress has been avoiding the underscore for private functions as they can often change status.
| function _wp_get_excluded_comment_types_clause( $column = 'comment_type' ) { | |
| function wp_get_excluded_comment_types_clause( $column = 'comment_type' ) { |
Requires changes elsewhere...
There was a problem hiding this comment.
oh, I had no idea we were changing that! THANKS!
There was a problem hiding this comment.
Renamed to wp_get_excluded_comment_types_clause() in 94dc58f.
Trunk moved the singular comment feed query below the post status checks (r-791c7fc07e). Re-apply the excluded comment types clause to the relocated block. Claude-Session: https://claude.ai/code/session_01RmMwj4CN9cF8b1Sx58Pvik
…ed-comment-types-filter # Conflicts: # src/wp-admin/includes/class-wp-comments-list-table.php # src/wp-admin/includes/comment.php # src/wp-includes/class-wp-comment-query.php # src/wp-includes/class-wp-query.php # src/wp-includes/comment.php # tests/phpunit/tests/query/commentFeed.php
…core. The column passed to the clause helper is an identifier, so prepare it with %i rather than interpolating it. A table-qualified name is split on the dot and each part quoted on its own, since `wp_comments.comment_type` as one quoted identifier would name a column that does not exist. Core is moving away from underscore-prefixed private functions, as they tend to become public later, so rename the helper to wp_get_excluded_comment_types_clause(). See #65537. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vwYjUikEZMVw2seRAHMXe
Changing the get_page_of_comment() and get_comment_link() defaults from 'all' to '' made the two values count differently, so a theme that copied the old default into its own call would keep counting excluded types. Walker_Comment does exactly that: it forwards the wp_list_comments() arguments, whose 'type' is 'all', so the comment permalinks rendered in the list still pointed at the wrong page. Restore the 'all' default and leave the default-excluded types out of the count when 'all' is requested instead. comments_template() never renders those types, so counting them only ever moved a permalink to a page the visitor does not see. A type requested by name is still counted. See #65537. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vwYjUikEZMVw2seRAHMXe
Use MockAction for the filter spies, assertSameSets() where order does not matter, and a message on every assertion in tests with more than one. Drop the remove_filter() calls the base test class already handles, and cover the quoting of the excluded types clause. See #65537. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vwYjUikEZMVw2seRAHMXe
Trunk renamed the shared test comment helper and now excludes reactions by default alongside notes.
…es-filter' into pr12310-feedback # Conflicts: # tests/phpunit/tests/comment/query.php
…es-filter' into pr12310-feedback # Conflicts: # tests/phpunit/tests/comment/query.php
|
Thanks for the review @peterwilsoncc! The inline notes are all addressed now, and you were right to be worried about Claude dug into how that change actually played out, here is what it found:
Does that approach work for you? It keeps the defaults untouched, the only behavior change is that |
…ed-comment-types-filter
Trunk's last-modified query hard-codes the 'note' and 'reaction' exclusion, so a type added through the 'default_excluded_comment_types' filter still moved the comment feed's Last-Modified date, and emptying the filter did not bring notes back. Route it through wp_get_excluded_comment_types_clause() like the feeds and counters. The cached value keeps its fixed 'lastcommentmodified:<tz>' key, since the comment status handlers clear it by that name. A changed excluded set is therefore picked up the next time a comment changes status. See #65537, #35214.
The docblocks and inline comments around the excluded comment types had grown into design notes, some of them stale: the accessor and filter docs still described 'note' as the only built-in default and did not mention 'reaction' or get_lastcommentmodified(). Trim them to what a caller needs and bring them in line with trunk. Also drop the list table's 'comment'/'comments' alias branch, which could never subtract anything because the accessor strips those tokens, and filter the registry's internal type names to strings so the merged default set is typed for array_unique(). See #65537, #35214.
New test methods and data providers need native types to pass PHPStan at level 10. See #65537.
Trunk is in the 7.2 beta, so these additions can no longer ship in 7.2.0.
Stack: Custom Comment Types (#35214)
register_comment_type()registration API/wp/v2/comment-typesendpointmap_meta_cap()render_callbackdisplay callbackis_pingping groupingdefault_excluded_comment_typesfilter (this PR)wp_update_comment_counts()recount helper#12311 and #12310 are on WordPress/wordpress-develop; the rest are fork PRs targeting their parent branch and will retarget to trunk as their base lands.
I asked Claude to tighten up this description, here it is:
Trac ticket: https://core.trac.wordpress.org/ticket/35214, https://core.trac.wordpress.org/ticket/65537
AI Use
Claude Code drafted the code and this description, and I steered. I will review and test.