Repository navigation
Comments: Notify users mentioned in a note - #12548
adamsilverstein wants to merge 21 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the 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. |
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. |
Port the notification layer from the Gutenberg Notes @mention work. On rest_insert_comment for note comments, parse the user-N mention classes out of the saved content, email each mentioned user plus any existing followers of the thread, and subscribe the note author and everyone they mention. Followers are stored as one _wp_note_followers meta row per user on the thread's top-level note so concurrent replies cannot clobber each other, and the meta is registered for REST so follower management UI can build on it. The post author is excluded because wp_new_comment_via_rest_notify_postauthor() already notifies them of every note, the note's own author is never notified about their own note, and recipients are limited to users who can edit_comment the note so note content cannot leak. Everything honors the wp_notes_notify option. See related Gutenberg pull request: WordPress/gutenberg#79606. Props mamaduka. Fixes #65639.
fc3e164 to
859ba08
Compare
Drop the per-thread followers model - the follower getters and setters, the `_wp_note_followers` meta and its registration, the follower union in the recipient set, and the `$was_mentioned` argument on the email filters. Subscription semantics are a separate concern from notifying a mentioned user, and should not ride along with this feature. Match the `<span class="wp-note-mention user-N">` chip markup that landed with the kses allowance; the parser still looked for anchors, so it found no mentions and sent no email. Also drop the notification subject and text filters, leaving the recipients filter as the single extension point. Follow-up to the Gutenberg source PR WordPress/gutenberg#79606.
Existing hooks already cover the cases it was added for. The long-standing `comment_notification_recipients` filter runs on every note through wp_notify_postauthor(), so extra recipients can be added there; `pre_wp_mail` and `wp_mail` can suppress or reroute delivery to another channel; and wp_send_note_notification() is public, so a plugin can notify anyone it likes from its own `rest_insert_comment` callback. That leaves this feature with no new filters at all.
Mamaduka
left a comment
There was a problem hiding this comment.
Looks good and I've already reviewed same code in Gutenberg repo. But would appriciate second opinion from @westonruter.
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
The post title and the site title were decoded on assignment and again when the assembled subject reached wp_mail(), resolving entities the author meant to be read literally. Note content was also decoded before its markup was stripped, so escaped text such as "<code>" was treated as a tag and dropped from the message body.
There was a problem hiding this comment.
Note on provenance: @westonruter asked Claude (Anthropic's Claude Opus 5, via Claude Code) to review this PR, and is posting the output here unedited. Findings were checked against a local
wordpress-developcheckout at the PR head — theTypeErrorbelow was reproduced by running PHPUnit, not inferred. Treat it as a machine-assisted review: useful signal, but please verify before acting.
Overview
Backports the mention-notification layer from Gutenberg #79606. Three new public functions in src/wp-includes/comment.php, one add_action() in default-filters.php, and a 406-line test file. On rest_insert_comment for note comments, mentions (<span class="wp-note-mention user-N">) are parsed out of the saved content and each mentioned user is emailed, minus the note author and the post author.
The design is sound and well-documented. The audience rules are right: the edit_comment gate matches WP_REST_Comments_Controller::check_read_permission() for notes (class-wp-rest-comments-controller.php:1920-1942 — author-or-edit_comment), and map_meta_cap() resolves edit_comment to edit_post of the note's post (capabilities.php:554-585), so no leak to subscribers. Tests pass (14, not the 12 the description claims), PHPCS is clean.
Blocking issue (Resolved by e5dcf46 ✅ )
Fatal TypeError from the WP_Comment type hint on a hook callback
comment.php:2620 — function wp_notify_note_mentions( WP_Comment $comment, ... ).
The controller fires the hook with get_comment( $comment_id ) (class-wp-rest-comments-controller.php:806-819), which returns null if the row is gone by then. Two reachable paths:
- Core-only:
handle_status_param()runs before theget_comment()at line 806. Withstatus=trash(noenumon the schema —sanitize_keyonly, line 1612),wp_trash_comment()hard-deletes whenEMPTY_TRASH_DAYSis0(comment.php:1609-1612).POST /wp/v2/comments {type: note, status: trash}on such a site →nullat the hook. - Any plugin on
wp_insert_commentthat deletes the comment.
Path 2 confirmed end-to-end with a throwaway PHPUnit test against this branch:
TypeError: wp_notify_note_mentions(): Argument #1 ($comment) must be of type WP_Comment, null given,
called in /var/www/src/wp-includes/class-wp-hook.php on line 353
/var/www/src/wp-includes/comment.php:2620
/var/www/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php:819
The sibling callback on the same hook handles this correctly and no-ops:
function wp_new_comment_via_rest_notify_postauthor( $comment ) {
if ( $comment instanceof WP_Comment && 'note' === $comment->comment_type ) {So this is a regression in robustness relative to the code right above it. The PR description says "the WP_Comment check moved from the signature into the function body" — that is what should be there, but the code does the opposite. Suggested fix:
function wp_notify_note_mentions( $comment, $request = null, $creating = true ) {
if ( ! $creating || ! ( $comment instanceof WP_Comment ) || 'note' !== $comment->comment_type ) {
return;
}This generalizes: hook callbacks should not carry hard class type hints, because do_action() has no contract enforcement and a TypeError in WP_Hook::apply_filters() is unrecoverable. Same reasoning applies to bool $creating (a caller passing null for a non-nullable scalar throws in coercive mode).
Should fix before landing
Missing filters
wp_send_note_notification() offers no filters at all, while every comparable core function does — wp_notify_postauthor() has comment_notification_recipients, comment_notification_subject, comment_notification_text, and comment_notification_headers (pluggable.php:1779, 1867, 1972, 1982). The PR argues pre_wp_mail/wp_mail cover it, but those are blunt: you cannot adjust this email's subject without string-matching, and comment_notification_recipients runs on the post-author path, not this one. At minimum a note_mention_notification_text/_subject pair would match the established shape and cost nothing. This is the point most likely to draw committer pushback.
Editor link is built with the sender's capabilities
comment.php — $edit_link = $post ? get_edit_post_link( $post->ID, 'url' ) : '';
Everything else in this function is composed in the recipient's context (switch_to_user_locale( $user->ID )), but get_edit_post_link() reads current_user_can(). The test file has to work around this explicitly:
/*
* The editor link comes from get_edit_post_link(), which is scoped to
* the current user; in the REST flow that is the note's author.
*/
wp_set_current_user( self::$commenter->ID );In the REST path this is benign — note creation already requires current_user_can( 'edit_post' ) (controller line 563) — but the function is public API, so any non-REST caller silently produces link-less emails. test_mentioned_user_is_emailed() in fact exercises that case without noticing. Gate on the recipient instead:
$edit_link = ( $post && user_can( $user->ID, 'edit_post', $post->ID ) )
? admin_url( 'post.php?post=' . $post->ID . '&action=edit' )
: '';Minor
- No email headers.
wp_notify_postauthor()sendsContent-Type: text/plain; charset=<blog_charset>(pluggable.php:1852-1853). This sends none, sowp_mail()'s defaults apply. Functionally fine, but inconsistent with the sibling notification. @param mixed $request— inconsistent with the strictly-typed$comment, and the hook documents it asWP_REST_Request. Should beWP_REST_Request $requestin the docblock (untyped in the signature).- Silent gap for a mentioned post author. The post author is skipped on the assumption
wp_new_comment_via_rest_notify_postauthor()covers them — but that path is gated by thenotify_post_authorfilter (comment.php:2533). If a plugin returnsfalsethere, a mentioned post author gets nothing at all. Worth a sentence in the inline comment at minimum. class_list()is case-sensitive in no-quirks mode, soUSER-5wouldn't match/^user-(\d+)$/. The markup is core-generated, so this is theoretical —preg_match( '/^user-(\d+)$/i', ... )would close it for free.- No length cap on
$content— a long note is emailed verbatim.
Process
- Stale PR description. It claims "no scalar type hints or return types" and "the
WP_Commentcheck moved from the signature into the function body" — the code hasstring $content,: array,: void,: bool,?WP_Post, and the check in the signature. It also says 12 tests; there are 14. Worth refreshing before a committer reads it. - Version timing.
@since 7.1.0against a trunk at7.1-beta4-src. New public API and new translatable strings after Beta 1 need an explicit exception from the release squad, or a bump to 7.2.
Verdict
Good, careful work — the capability gating in particular is correct and well-justified, and the test coverage (including two real REST dispatches guarding the hook wiring) is above average for a change this size. The type hint on the hook callback is a genuine fatal and should be removed before this lands. The missing filters are the main thing a core committer is likely to hold it on.
…immediately deleted
|
I'm fine adding the suggested filters, the argument for not adding them was to reduce new public APIs when we know a new full blown notification system is needed. I went back and forth on it when working on this. What do you think @westonruter? |
|
IMO, the filters can be added later as need arises. |
This makes sense to me to add. We wouldn't want HTML email to be sent accidentally, right? |
This review also makes sense to me. Unless the functions get marked as private, it should probably make sure that the edit post link is constructed appropriately for the recipient. Also consider the case where the functions could be called during WP Cron when no user is logged-in, in which case no edit link would be included. |
right
Good point, will adjust. |
The message is composed for the recipient, but the editor link was not: get_edit_post_link() answers for whoever is current, which is the note's author over REST and nobody at all under WP-Cron, so the link was either built against the wrong capabilities or dropped entirely. Switch to the recipient for the length of that call. Also declare the plain text content type explicitly, matching wp_notify_postauthor(), so a filtered wp_mail() default cannot turn the message into HTML.
|
Both addressed in 6b687d1. Content type. Now sends Editor link. Switches to the recipient for the length of the $edit_link = '';
if ( $post ) {
$previous_user_id = get_current_user_id();
wp_set_current_user( $user->ID );
$edit_link = (string) get_edit_post_link( $post->ID, 'url' );
wp_set_current_user( $previous_user_id );
}Good call on WP-Cron - I wrote a test that sets the current user to 0 before notifying, and against the old code it failed with no link in the message at all. Two new tests cover both changes ( Worth noting On the filters - agreed, leaving them out and adding later as the need arises. |
|
Fixed in https://core.trac.wordpress.org/changeset/63012. cc: @t-hamano |
Backports the notification layer from the Gutenberg Notes
@mention work (WordPress/gutenberg#79606, still open upstream). Split out of #12503, which covered the kses allowance for the mention markup.What
A user who is
@mentioned in a note gets an email about that note. That is the whole feature.This is a straight port, deliberately not a superset: the same three functions, the same audience rules, the same twelve tests. The only differences from the plugin copy are core conventions -
wp_prefixes, no scalar type hints or return types, no text domain,@vardocblocks on the test properties, and theWP_Commentcheck moved from the signature into the function body. An earlier revision of this PR also carried a per-thread followers model and a deep link into the specific thread; both were dropped upstream and are not here.On
rest_insert_commentfornotecomments - alongside the existing post-author notification -wp_notify_note_mentions()parses the mentions out of the saved content and emails each mentioned user.Mentions are stored as chips carrying the mentioned user's ID in a class token:
<span class="wp-note-mention user-N">@Name</span>. Only elements carrying both classes are treated as mentions. The email is composed in the recipient's locale viaswitch_to_user_locale()and links to the post editor withget_edit_post_link(), the same waywp_notify_postauthor()does for the post author's note email.Audience rules:
wp_new_comment_via_rest_notify_postauthor()already notifies them of every note, so including them here would send a duplicate.edit_commentthe note, matchingWP_REST_Comments_Controller::check_read_permission()for notes, so emails cannot leak note content to users who cannot see the note in the editor.wp_notes_notifyoption.New public API
Three functions, no new filters:
wp_get_note_mentioned_user_ids( $content )wp_notify_note_mentions( $comment, $request, $creating )wp_send_note_notification( $user, $comment, $post )Existing hooks cover extension:
comment_notification_recipientsalready runs on every note throughwp_notify_postauthor(), so extra recipients can be added there;pre_wp_mailandwp_mailcan suppress or reroute delivery to another channel; andwp_send_note_notification()is public, so a plugin can notify anyone it likes from its ownrest_insert_commentcallback.Running alongside the Gutenberg plugin
The plugin ships the same feature in its
lib/compat/wordpress-7.1/layer until this lands, so with both activerest_insert_commentwould carry two mention callbacks and email every mentioned user twice. The dedupe belongs on the plugin side, since core has no way to know about the plugin, and it is already in place upstream: the compat file removeswp_notify_note_mentionsand keeps its own copy, matching how the plugin handles other backported hooks. Core'swp_new_comment_via_rest_notify_postauthor()is deliberately left attached - the mention path never emails the post author, so those two do not overlap. Nothing to do in this PR; noting it so the coexistence story is written down somewhere.Testing
tests/phpunit/tests/comment/wpNotifyNoteMentions.phpcovers mention parsing (non-mention spans and plain anchors ignored, IDs deduped), the mentioned-user email, email content and the editor link, author self-exclusion, post-author exclusion, capability gating, nonexistent-user mentions, thewp_notes_notifyoff switch, and - through real REST dispatches - that creating a note viaPOST /wp/v2/commentstriggers the email while updating one does not, which guards therest_insert_commentwiring itself.12 tests pass locally.
Manual steps:
@mention the second user.Commit message