Make WordPress Core

Opened 8 years ago

Last modified 2 days ago

#40477 assigned defect (bug)

REST API: Does NOT Trigger New User Notifications!

Reported by: mrahmadawais's profile mrahmadawais Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.9 Priority: normal
Severity: normal Version: 4.7
Component: Users Keywords: dev-feedback has-patch needs-testing has-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 8 years ago.
ADD: Email Notifications on user creation by WP REST API
40477.2.patch (649 bytes) - added by mrahmadawais 8 years ago.
ADD: Email Notifications on user creation by WP REST API
40477.3.patch (800 bytes) - added by mrahmadawais 8 years ago.
ADD: Email Notifications on user creation by WP REST API
40477.diff (1.2 KB) - added by nikunj8866 12 months ago.
ADD: Hook to manage Email Notifications on user creation by WP REST API

Download all attachments as: .zip

Change History (39)

#1 @swissspidy
8 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
8 years ago

ADD: Email Notifications on user creation by WP REST API

#2 @mrahmadawais
8 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.


8 years ago

#4 @websupporter
8 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 8 years ago by websupporter (previous) (diff)

@mrahmadawais
8 years ago

ADD: Email Notifications on user creation by WP REST API

#5 @mrahmadawais
8 years ago

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

#6 @swissspidy
8 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
8 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
8 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
8 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
8 years ago

ADD: Email Notifications on user creation by WP REST API

#10 follow-up: @mrahmadawais
8 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.


8 years ago

#12 in reply to: ↑ 10 @jnylen0
8 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
12 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
12 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
12 months ago

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

#16 @colorful tones
12 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
12 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
12 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
9 months 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?

This ticket was mentioned in Slack in #core by nikunj8866. View the logs.


9 months ago

#21 @colorful tones
9 months ago

Tested patch 40477.diff and verified it is working.

Testing scenario:

curl --user "test_user:loUEbMZDT4Cf3cZQNy6K7CDi" \
     -X POST \
     -H "Content-Type: application/json" \
     -d '{"username":"larry","password":"secret","email":"larry@example.com"}' \
     http://localhost:8889/wp-json/wp/v2/users

I can not speak to whether the code introduced is ideal, but it addresses the issue. Thanks for the patch @nikunj8866

This ticket was mentioned in Slack in #core by colorful-tones. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by mikachan. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


3 months ago

This ticket was mentioned in Slack in #core by benjamin_zekavica. View the logs.


5 weeks ago

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


5 weeks ago

#27 @joemcgill
5 weeks ago

  • Milestone changed from Awaiting Review to 6.9
  • Owner set to spacedmonkey
  • Status changed from new to assigned

Moving this to he 6.9 milestone for @spacedmonkey to review.

@nikunj8866 if it is possible for you to apply your latest patch to a new pull request to our GitHub mirror, that will help in making the review process smoother. Instructions can be found at https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/.

Thanks all!

@nikunj8866 commented on PR #8572:


5 weeks ago
#29

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 props-bot label.

## Unlinked Accounts
The following contributors have not linked their GitHub and WordPress.org accounts: @nhatkar@….

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

I want to use @nikunj8866 WordPress.org account for this PR.

This ticket was mentioned in PR #8672 on WordPress/wordpress-develop by @abcd95.


2 weeks ago
#30

  • Keywords has-unit-tests added; needs-unit-tests removed

Trac ticket: https://core.trac.wordpress.org/ticket/40477

This PR adds unit tests verifying email notifications are sent when creating users through the REST API. Tests cover:

  • Default admin notification behavior
  • Filter override capability via rest_wp_user_created_notification
  • Multi-recipient notifications when using 'both' parameter

Follows the implementation from https://github.com/WordPress/wordpress-develop/pull/8572 by @nikunj8866.

@abcd95 commented on PR #8672:


2 weeks ago
#31

The tests fail only in the multisite tests since, in those environments, we also need Super Admin privileges to create users. I will update the tests accordingly.

@abcd95 commented on PR #8572:


7 days ago
#32

@nikunj8866 After investigation, I found that the notification is triggered in the multisite environments behaves differently, and an extra email is being triggered, which is failing the tests.
https://github.com/WordPress/wordpress-develop/actions/runs/14440085345/job/40488175505?pr=8672

Can you please test if this change accounts for the multisite environments as well? Since there, the superadmin privileges also come into play.

@nikunj8866 commented on PR #8572:


7 days ago
#33

@himanshupathak95 I've updated the PR to ensure that registration notifications are sent to both the user and the admin. The implementation now aligns with how WordPress core handles this for both single-site and multisite environments.

Specifically, I’m following the same approach as used in core:
https://github.com/WordPress/wordpress-develop/blob/ec5bf32bd23df7abe100e1b8891f8d9be8685c07/src/wp-includes/default-filters.php#L524
https://github.com/WordPress/wordpress-develop/blob/ec5bf32bd23df7abe100e1b8891f8d9be8685c07/src/wp-includes/ms-default-filters.php#L30
https://github.com/WordPress/wordpress-develop/blob/ec5bf32bd23df7abe100e1b8891f8d9be8685c07/src/wp-includes/user.php#L3587

As you noted, the behavior does vary in multisite environments due to the presence of super admin roles and additional email triggers. Since the native /wp/v2/users REST API endpoint functions on a per-site basis and not network-wide, this difference might be expected.

If this test failure is strictly due to multisite behavior, perhaps we can either skip the test for multisite scenarios or open a discussion in the core regarding how such notifications should behave across different environments.

@nikunj8866 commented on PR #8572:


7 days ago
#34

@himanshupathak95 Password Changed email additional triggered for multisite.

https://github.com/user-attachments/assets/cc1c8029-556e-4cc9-96e2-28d207d69541

@abcd95 commented on PR #8572:


2 days ago
#35

@himanshupathak95 Password Changed email additional triggered for multisite.

Thanks @nikunj8866 for following up and testing this out.

However, ever after applying the latest commit, the tests are still failing. I am investigating this on my end.

Note: See TracTickets for help on using tickets.