Make WordPress Core

Opened 7 years ago

Last modified 15 hours 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 (4)

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
40477.diff (1.2 KB) - added by nikunj8866 3 months ago.
ADD: Hook to manage Email Notifications on user creation by WP REST API

Download all attachments as: .zip

Change History (23)

#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
7 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.

#14 @philsola
3 months ago

Hi,

I'm coming up against this exact issue myself now, 7 years on from when this ticket was opened.

I can see 3 patches were suggested at the time, and no decisions made and this functionallity is still missing.

Is there any more ideas on how we can move this forwards and get something implemented in core?

It would be really great to see this finally resolved in core.

#15 @colorful tones
3 months ago

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.

And

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.

These sound like a clear way forward. @philsola do you think you would be interested in submitting a patch that adds a new filter to send a notification and plugins can override it?

@nikunj8866
3 months ago

ADD: Hook to manage Email Notifications on user creation by WP REST API

#16 @colorful tones
3 months ago

Thanks for the patch @nikunj8866 ! @philsola do you mind giving it a test please, and let us know how it works for you?

#17 @colorful tones
3 months ago

@swissspidy perhaps you could consider testing and providing your insight as well please? It might be a nice candidate for the upcoming WP 6.6 depending on how many contributors there are and release squad capacity. Thanks all!

#18 follow-up: @philsola
3 months ago

@colorful-tones Thank you so much for jumping in on this today, that's great! Sorry for the delayed reply, it's been a hectic day and I'm away for the weekend now, but will be able to test it first thing on Monday and report back if that works?

Thanks @nikunj8866 for the patch! Greatly appreciated.

Agree with @colorful-tones it would be great to get this in for 6.6 if possible.

Thanks again!

#19 in reply to: ↑ 18 @nikunj8866
15 hours ago

Replying to philsola:

@colorful-tones Thank you so much for jumping in on this today, that's great! Sorry for the delayed reply, it's been a hectic day and I'm away for the weekend now, but will be able to test it first thing on Monday and report back if that works?

Thanks @nikunj8866 for the patch! Greatly appreciated.

Agree with @colorful-tones it would be great to get this in for 6.6 if possible.

Thanks again!

@philsola @colorful-tones

Just checking in to see if you had a chance to test it. Could you please provide an update on the status when you get a moment?

Note: See TracTickets for help on using tickets.