Make WordPress Core

Opened 7 years ago

Last modified 6 years ago

#40477 new defect (bug)

REST API: Does NOT Trigger New User Notifications!

Reported by: mrahmadawais's profile mrahmadawais Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7
Component: Users Keywords: dev-feedback has-patch needs-testing needs-unit-tests
Focuses: rest-api Cc:

Description

If you create new users with WP REST API. The notification for new WordPress users via email does NOT get triggered. I tried it on a fresh install. Used the [Email log](https://wordpress.org/plugins/email-log/) WP plugin to test that no emails were sent.

Attachments (3)

40477.patch (682 bytes) - added by mrahmadawais 7 years ago.
ADD: Email Notifications on user creation by WP REST API
40477.2.patch (649 bytes) - added by mrahmadawais 7 years ago.
ADD: Email Notifications on user creation by WP REST API
40477.3.patch (800 bytes) - added by mrahmadawais 7 years ago.
ADD: Email Notifications on user creation by WP REST API

Download all attachments as: .zip

Change History (16)

#1 @swissspidy
7 years ago

  • Component changed from General to Users
  • Keywords dev-feedback added
  • Summary changed from WP REST API — Does NOT Trigger New User Notifications! to REST API: Does NOT Trigger New User Notifications!
  • Version changed from 4.7.3 to 4.7

Related: #40352.

@mrahmadawais
7 years ago

ADD: Email Notifications on user creation by WP REST API

#2 @mrahmadawais
7 years ago

  • Keywords has-patch needs-testing added

@swissspidy

Added the ticket after discussion with @rmccue

I just created a patch which is now triggering notifications. Calling wp_new_user_notification(); did the trick.

Looking forward!

This ticket was mentioned in Slack in #core-restapi by mrahmadawais. View the logs.


7 years ago

#4 @websupporter
7 years ago

Hi @mrahmadawais,

Maybe we should use wp_send_new_user_notifications() and trigger it after is_wp_error? What do you think?

Shouldn't this also apply to users created in a multisite or is newuser_notify_siteadmin() hooked into wpmu_create_user() sufficient?

Last edited 7 years ago by websupporter (previous) (diff)

@mrahmadawais
7 years ago

ADD: Email Notifications on user creation by WP REST API

#5 @mrahmadawais
7 years ago

@websupporter Thanks for pointing out another function. It does what I was doing. Updated the patch.

#6 @swissspidy
7 years ago

  • Keywords needs-unit-tests added

wp_send_new_user_notifications() is added to a few hooks, for example add_action( 'register_new_user', 'wp_send_new_user_notifications' );. 40477.2.patch means one cannot opt-out of this.

#7 @TimothyBlynJacobs
7 years ago

Yeah, I think this should be attached to a hook of some kind. Ideally, this would also be controlled by a param in the POST body. This would also match WP Admin's add new user form.

#8 @websupporter
7 years ago

Yes, after doing some research, I think so too. Actually, I think, we should use 'register_new_user' like in user.php:

/** This action hook is documented in wp-includes/user.php **/
do_action( 'register_new_user', $user_id );

Ref.: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/user.php#L2266

So, if someone dehooked the notice for the normal user registration (on wp-login.php), the notice would also not be send via the REST API. But maybe to register a user via the REST API can be seen as a new action seperate of the user registration on wp-login.php in which case a new action would be sensible.

Or, actually this convinces me more, we see a registration via the REST API more aligned with the action in the wp-admin area. In this case, we should probably use
'edit_user_created_user'.

/** This action hook is documented in wp-admin/includes/user.php **/
do_action( 'edit_user_created_user', $user_id, $notify );

Ref.: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/user.php#L197

The $notify in the admin is populated via $_POST. If you mark the checkbox to send the password to the new user $notify will be 'both', otherwise it will be 'admin'.

I was thinking about @TimothyBlynJacobs comment

Ideally, this would also be controlled by a param in the POST body.

I think, this would be actually nice, since this means, we could easily "Restify" the "Create New User"-form in the backend 1:1. But I can't figure out a param which on the other side meets the criteria to be general enough for a API param. Somehow I dislike a notify parameter here.

Right now all params are somehow information about the new user (name, email, locale etc.). A notify-param is not an additional information but triggers an action and breaks with the existing schema.

Maybe someone comes up with a better idea or my concerns are somehow mislead here, but if not:

A notify-param would switch the 'admin'/'both'-behavior (if we go with the current behavior). If we are not using such a param, we would need to decide, which value $notify should have.

Coming from the original problem of this ticket, I think we should use both, otherwise, we would miss the topic :)

The notification for new WordPress users via email does NOT get triggered.

This would be also inline with the standard behavior of wp_send_new_user_notifications().

#9 @mrahmadawais
7 years ago

@swissspidy Makes sense.

I agree with what @websupporter said, we can have a notify param that could be set with the REST API. If not set, it could default back to the default WP behavior.

@mrahmadawais
7 years ago

ADD: Email Notifications on user creation by WP REST API

#10 follow-up: @mrahmadawais
7 years ago

I have updated the patch to use

/*
 * Trigger default welcome email notifications.
 *
 * This action hook is documented in wp-admin/includes/user.php
 *
 * Add second param $notify to override the behavior.
 */
do_action( 'edit_user_created_user', $user_id );

This will favor the default notification behavior as well as provide the ability to opt-out by defining a custom value for $notify as the second argument.

This ticket was mentioned in Slack in #core-restapi by mrahmadawais. View the logs.


7 years ago

#12 in reply to: ↑ 10 @jnylen0
7 years ago

Replying to mrahmadawais:

I have updated the patch to use 'edit_user_created_user'.

This will favor the default notification behavior as well as provide the ability to opt-out by defining a custom value for $notify as the second argument.

In 40477.3.patch I'm not seeing any functionality in place to specify $notify. What do you mean by this?

We discussed a bit more in Slack, and haven't reached a conclusion yet. While I recognize the desire for this functionality, I'm not a big fan of adding a parameter like notify because it's not RESTful (it doesn't REpresent the State of an object).

A potential solution would be adding a new filter like rest_user_created_notification that plugins can override. Until then, you can use register_rest_field to add a suitable parameter here.

I also wish that we had enabled this behavior by default, for both users and comments.

#13 @squarecandy
6 years ago

This can currently be accomplished without core modification in this way:

<?php
function mynamespace_user_update($user, $request, $create)
{
  $user_id = $user->ID;
  wp_send_new_user_notifications($user_id);
}
add_action( 'rest_insert_user', 'mynamespace_user_update', 12, 3 );

But, yeah, a patch to make this happen by default seems like it would be nice, especially if there's also a hook to modify it to not send the messages.

Note: See TracTickets for help on using tickets.