Notification Functions Should be Replaced by Hooks

[dshanske]

wp_notify_postauthor, wp_notify_moderator, wp_new_user_notification, wp_password_change_notification should be replaced by hooks.

Basically, I am proposing removing the code from these functions to a secondary function. These functions, which are pluggable and needed for legacy reasons, would remain, but would just have the hook inside. This would ensure backward compatability.

Not only does, in my opinion, make more sense going forward, it removes the assumption that a notification is an email. Today, it could be a text message, a push notification to a phone, so on.

[atomicjack]

+1

Possibly paving the way for different notification types? (re: your last sentence)

[pfefferle]

I would love to see the same filters/hooks for the notifications that are used by the views. For example:

$notify_message .= sprintf( __( 'Comment: %s' ), "\r\n" . apply_filters( 'get_comment_text', $comment->comment_content, $comment, $args ) ) . "\r\n\r\n";

or at least some compatible filters like:

apply_filters( 'get_comment_notification_text', $comment->comment_content, $comment, $args ) )

[dshanske]

Replying to atomicjack:

+1

Possibly paving the way for different notification types? (re: your last sentence)

That would be one advantage, yes. The medium of communication doesn't have to be in core, but core can trigger it.

[dshanske]

Patch for Adding Hooks to Notify

[dshanske]

First patch I've ever tried to do.

[boonebgorges]

dshanske - Thanks for the idea, and for the patch! I agree that it'd be nice to handle these via hooks, if only so that they can be easily removed.

A general question about strategy. You've torn the guts from the existing functions and replaced those guts with do_action() calls. IMO, it makes more sense to replace the direct calls to wp_notify_postauthor(), etc with do_action() calls. In some cases, you may need very small wrappers. See [33587.2.diff] for a suggested technique. This has the advantage of making the notifications unhookable, but greatly reduces the amount of churn (and new function names) that your patch requires. And if you don't like the idea of a wrapper like wp_send_comment_notifications(), we could introduce new actions inside of wp_new_comment() - maybe 'comment_post_not_approved' and 'comment_post_author_notify'.

Is there a reason why this sort of thing wouldn't work?

Some more general notes about the patch:

• As mentioned in Slack, it makes testing much easier if the patch is generated from the WordPress root.
• In the absence of extenuating circumstances, new function names should start with wp_.
• Tabs not spaces for indentation.
• When using add_action() or add_filter(), no need to pass 1 as the fourth param ($accepted_args). 1 is assumed as default.
• All calls to add_action() and add_filter() should go in default-filters.php (or ms-default-filters.php, or wp-admin/includes/admin-filters.php, or wp-admin/includes/ms-admin-filters.php, as appropriate).

[dshanske]

Well, you are right that it is probably better to put the hook further up in the process.

The part of the guts of the functions I do think should be ripped out is the part that generates the comment information to be placed in the notification. This is duplicative between wp_notify_postauthor and wp_notify_moderator and would be better served if the same code did both. Also...if you note pfefferle's comment, the part you might want to enhance the display of.

[boonebgorges]

I think it would be great to reduce duplication in these functions, but let's try to do one thing at a time :)

[dshanske]

Add Hooks

[dshanske]

Okay, tried the suggested approach.

[dshanske]

Moved the second part of the suggestion to #33735

[johnbillion]

+1,000.

Docs need tidying up a bit but this is good. Moving to 4.4 for consideration.

[thomaswm]

Tidied up inline documentation

[thomaswm]

Replying to johnbillion:

Docs need tidying up a bit but this is good.

I tried to do this in 33587.2.diff​.

There are some more notification functions in multisite, for example wpmu_signup_user_notification, wpmu_new_user_notification etc. Should those be replaced by hooks, too?

[boonebgorges]

These patches are looking good, but I don't like the action names being suggested. Action/filter names ought to describe the context of the action/filter, not the intended use. Also, I think we can make the code flow a bit more logically by using the existing 'comment_post' hook and introducing small wrapper functions that do the weird logic from the end of wp_new_comment(). See [33587.3.diff].

There are some more notification functions in multisite, for example wpmu_signup_user_notification, wpmu_new_user_notification etc. Should those be replaced by hooks, too?

Yeah, let's do it. Note that those add_action() calls should go in ms-default-filters.php.

[thomaswm]

33587.4.diff​ also takes care of

• wpmu_welcome_user_notification
• wpmu_welcome_notification
• wpmu_signup_user_notification
• wpmu_signup_blog_notification

Also, I have replaced some more calls of wp_new_user_notification which I found in files in wp-admin/network/.

[dshanske]

I didn't even think of the multisite stuff. Apologies.

I've learned a few things doing this one, which is the first ticket I've filed and tried to patch. Also the first issue that looks like it is going to get into core.

It is not going in as I've originally imagined, but I think it is better than I thought.

[jeremyfelt]

I love this ticket.

• wp_insert_user() has a user_register action that fires for new users. Could that be used for one or more of the create_user actions?
• I'd lean toward after_signup_site rather than after_signup_blog, even though it's in wpmu_signup_blog().

[dshanske]

The problem is that user_register will notify on any insertion of a user. There may be some specific circumstances where you wouldn't want to notify. It might need more logic.

[boonebgorges]

This is getting close - thanks all.

Two comments:

• I know I originally suggested it, but passing the string 'both' to a filter is really ugly. Let's have a new wrapper function, something like wp_send_new_user_notification( $user_id ), which will call wp_new_user_notification( $user_id, 'both' ).
• I haven't looked closely, but are we certain that we want to have the same hook name for all of these user creation locations? 'create_user' seems too general for me - at the very least, I'd like to see the action name describe more specifically what's happening at each of the points where it's called. (The fact that the exact same action is being suggested in wp-admin/network/user-new.php and in wp-admin/includes/user.php is a code smell to me.) Let's only duplicate action names if it really is the exact same thing that's happening in each spot. And if we do duplicate names, let's use the correct documentation formatting: ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#4-1-duplicate-hooks
• Related to the above, I think it should be researched whether 'user_register' could be used. If we need to introduce a new wrapper to handle a bit of the logic that's currently inline in the various places where the notification function is called, it's probably preferable to do that than to introduce new actions (because actions, once introduced, must be maintained forever). As a rule, actions are like signposts, and they should be put into places that are neither too specific nor too general. A good test is probably: if we're having a hard time coming up with a succinct name for the action, then maybe the action we're proposing is not a good one.
• What jeremyfelt said about 'after_signup_site' :)

[boonebgorges]

In 34106:

Send comment notification emails via a hooked function.

Previously, wp_notify_postauthor() and wp_notify_moderator() were called
directly from wp_new_comment(), making it difficult to modify or suppress
default notification emails.

Props dshanske, thomaswm.
See #33587.

[boonebgorges]

In 34107:

Send password-change email notifications via hook.

wp_password_change_notification() is now called at the 'after_password_reset'
action, rather than being invoked directly from the reset_password() function.

In order to make it possible to call wp_password_change_notification() as a
do_action() callback, the function signature has to be changed so that the
$user parameter is expected to be a value rather than a reference. Since
PHP 5.0, objects are passed by reference, so &$user was unnecessary anyway.

Props dshanske, thomaswm.
See #33587.

[boonebgorges]

In 34112:

Send multisite site/user signup emails via hooked functions.

Site and user signup notifications are moved to the new actions
'after_signup_site' and 'after_signup_user'. Site and user activation
notifications are moved to the existing actions 'wpmu_activate_blog' and
'wpmu_activate_user'.

Props dshanske, thomaswm, jeremyfelt.
See #33587..

[boonebgorges]

Hi all - So, most of the changes are in. The remaining work has to do with the notifications sent to the site admin and new user when the admin creates a user through the Dashboard. This happens in a bunch of places throughout wp-admin and wp-admin/network. thomaswm's suggestion was to introduce a single 'create_user' action in all the necessary places. But this seems like a bad action name, for reasons described above.

jeremyfelt suggested perhaps using the 'user_register' hook. On further thought, this can't be done, at least not in a simple way - there are too many ways that wp_insert_user() is used, and many of them shouldn't involve email notifications.

I've attached two patches with suggested solutions:

1. 33587.5.diff​ adds different action names in each place. The upside: this is pretty straightforward. The downside: it means placing a bunch of new action hooks in weird places. Putting do_action() calls into the business logic of files like user-new.php feels all sorts of wrong to me.
2. 33587.6.diff​ uses the 'user_register' hook. In order to make sure that email notifications are only sent when we want them, the patch adds a $notify param - defaulting to false - to wpmu_create_user(), wp_create_user(), and wp_insert_user(). The $notify value is then passed to the 'user_register' hook, and we send the email based on this value, in wp_maybe_send_new_user_notifications(). Then, in all places in core where we want to send notifications, we pass $notify = true. Upside: we don't have to introduce any new actions. Downside: It means adding a very ugly param to the wp_insert_user() stack. In addition, it'll be hard to target specific emails for unhooking or modifying, since they'll all run through 'user_register'; this sorta defeats the purpose of the ticket, which is to make it easier to modify specific email notifications.

Any thoughts on these two strategies?

[dshanske]

Both seem like they would create problems. The first in terms of complexity, the second in terms of usage.

Maybe the question is... what is the current logic that sends out the notifications? Why do some new users get notified, and others don't?

[boonebgorges]

Replying to dshanske:

Both seem like they would create problems. The first in terms of complexity, the second in terms of usage.

Maybe the question is... what is the current logic that sends out the notifications? Why do some new users get notified, and others don't?

Different notifications are sent depending on context. For example, on multisite with open registration, users are created when a signup is activated. But then the new user gets a "your account has been activated" message, not the standard "new user" message.

More generally, there are countless plugins etc that create users programatically, either directly through wp_insert_user(), or using a wrapper like wp_create_user() or wpmu_create_user(). They have come to expect that notifications will not be sent. We can't change this behavior.

The more I think about it, the more I think 33587.5.diff​ is the way to go. The new hooks are somewhat wonky, but no more wonky than the fact that we were manually calling the email notification functions at this point. Speak now or forever hold your peace.

[dshanske]

If there is a better way, it would break backward compatibility, which is a no-no. So it is the best, suppose.

[cfinke]

r34106 introduced a bug causing email notifications to be sent for comments that have been caught as spam.

Steps to reproduce (if you're running Akismet):

1. Ensure that you have checked the "Email me whenever [] anyone posts a comment" option in Settings > Discussion.
2. Leave a comment on a post you wrote, and use the author name viagra-test-123. This will guarantee a spam result from Akismet.
3. Check your WordPress Spam folder; the comment will be in there.
4. Check your email; you should have a "New comment on your post" email from the comment you left in step 2.

Patch forthcoming. Patch and steps to reproduce generated in cooperation with @kraftbj.

[cfinke]

Prevent notifications from being sent when the comment was caught as spam.

[boonebgorges]

Replying to cfinke:

r34106 introduced a bug causing email notifications to be sent for comments that have been caught as spam.

Steps to reproduce (if you're running Akismet):

1. Ensure that you have checked the "Email me whenever [] anyone posts a comment" option in Settings > Discussion.
2. Leave a comment on a post you wrote, and use the author name viagra-test-123. This will guarantee a spam result from Akismet.
3. Check your WordPress Spam folder; the comment will be in there.
4. Check your email; you should have a "New comment on your post" email from the comment you left in step 2.

Patch forthcoming. Patch and steps to reproduce generated in cooperation with @kraftbj.

Thanks for the patch. I can confirm that this was missed in [34106]. It only applies to wp_new_comment_notify_postauthor(), because the moderator check was already doing a check for '0', which is redundant with != 'spam'.

I'll try to write a unit test that reflects the problem.

[boonebgorges]

In 34250:

Don't notify post authors about spam comments.

[34106] moved post author notification to a hook, and in the process, missed
the 'spam' check. This changeset restores that check.

To make unit testing easier, the notification callbacks have been refactored
to return values: false when various conditions aren't met (eg, approved
comments should not trigger moderation emails), and the return value of the
wp_notify_*() function otherwise.

Props cfinke, kraftbj.
See #33587.

[SergeyBiryukov]

Is there a reason why wp_new_comment_notify_moderator() uses the passed $comment_approved argument, while wp_new_comment_notify_postauthor() uses get_comment() to get the same value? Shouldn't they both use the same method for consistency?

[boonebgorges]

In 34251:

Move new user notification emails to add_action() callbacks.

When a new user is created in various places throughout the interface,
notifications are sent to the site admin and the new user. Previously, these
notifications were fired through direct calls to wp_new_user_notification(),
making it difficult to stop or modify the messages.

This changeset introduces a number of new action hooks in place of direct calls
to wp_new_user_notification(), and hooks the new wrapper function
wp_send_new_user_notifications() to these hooks.

Props dshanske, thomaswm, boonebgorges.
Fixes #33587.

[boonebgorges]

Replying to SergeyBiryukov:

Is there a reason why wp_new_comment_notify_moderator() uses the passed $comment_approved argument, while wp_new_comment_notify_postauthor() uses get_comment() to get the same value? Shouldn't they both use the same method for consistency?

Only as a relic of the way the patch was originally written :-/ I'll change it.

[boonebgorges]

In 34252:

Improve consistency of comment notification callback signatures.

Both wp_new_comment_notify_moderator() and wp_new_comment_notify_postauthor()
now accept a single argument: $comment_ID.

Props SergeyBiryukov.
Fixes #33587.

[johnbillion]

#32563 was marked as a duplicate.

[johnbillion]

#11210 was marked as a duplicate.