Send email confirmation email on rest-api updates - #3813
Conversation
…o profile_update and one that processes the change.
…ess the email change.
a1e055c to
bc85780
Compare
|
Some additional unit tests of Strings, documentation, and function naming likely needs a second set of eyes. |
| if ( send_user_email_change_confirmation_process( $current_user->ID, $_GET['newuseremail'] ) ) { | ||
| wp_redirect( add_query_arg( array( 'updated' => 'true' ), self_admin_url( 'profile.php' ) ) ); | ||
| die(); | ||
| } else { | ||
| wp_redirect( add_query_arg( array( 'error' => 'new-email' ), self_admin_url( 'profile.php' ) ) ); | ||
| } | ||
| die(); |
There was a problem hiding this comment.
Worth noting that this code branch is no longer used, and is here primarily for existing emails that have been sent.
| if ( ! is_email( $email ) ) { | ||
| return new WP_Error( | ||
| 'user_email', | ||
| __( '<strong>Error:</strong> The email address is not correct.' ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm not sure these should be in this function, can probably be moved back into send_confirmation_on_profile_email(), or perhaps they should all be within edit_user() instead.
| } | ||
|
|
||
| if ( email_exists( $email ) ) { | ||
| delete_user_meta( $user->ID, '_new_email' ); |
There was a problem hiding this comment.
I'm not sure why this delete_user_meta() is present within this error message block.
| * @param WP_User $user The user having their email changed. | ||
| * @param string $email The new email address. | ||
| */ | ||
| $should_send_email_for_change = apply_filters( 'should_send_email_for_email_change', $should_send_email_for_change, $user, $email ); |
There was a problem hiding this comment.
This filter can be used instead of remove_action( 'personal_options_update', 'send_confirmation_on_profile_email' );.
// Skip email confirmation for user email changes.
add_filter( 'should_send_email_for_email_change', '__return_false' );
| $the_user = get_user_by( 'id', $user_id ); | ||
| $user = new stdClass(); | ||
| $user->ID = $the_user->ID; | ||
| $user->user_email = esc_html( trim( $new_email['newemail'] ) ); |
There was a problem hiding this comment.
I'm not sure why this is esc_html(), this seems somewhat in error to me, but it's what the existing code does.
| /** | ||
| * Fires an action hook when the account email has been confirmed by the user. | ||
| * | ||
| * @since x.x | ||
| * | ||
| * @param int $user_id User ID. | ||
| */ | ||
| do_action( 'user_email_confirmed', $user_id ); |
There was a problem hiding this comment.
This can be used for things like, redirecting to a custom page after the user confirms their email change - for example, to a custom WordPress (or bbPress) page.
…gle site and multisite.
|
I rebased this onto trunk at #13488. Your commits are the base there with authorship intact, so the diff against the first commit is only what I changed. I couldn't push here since the branch is on your fork. Two things needed fixing beyond the rebase. send_user_email_change_confirmation_process() uses $wpdb in the multisite branch but there's no global $wpdb;. The body came out of wp-admin/user-edit.php where $wpdb was already in scope at the top level, so confirming an email change on multisite calls a method on null. The PHPStan baseline already has Variable $wpdb might not be defined for user-edit.php at count 12, and moving this code out drops it to 6. The new function only escaped the same report because it was new. The other one is #44672. email_exists() isn't case sensitive, so putting the profile screen and REST through the same check means a user fixing the case of their own address gets told it's already in use. That's a 400 on REST and an error on the profile screen. test_update_item_existing_email_case fails on the patch as it stands. I made a case-only change not count as a change that needs confirming, which is what wp_update_user() already does with its own strcasecmp() guard. Smaller things. The split dropped the 0 === $current_user->ID || $current_user->ID !== (int) $user_id guard, so get_user_by( 'id', 0 ) returns false and fatals on PHP 8 when logged out. It also dropped the addslashes() restore of $_POST['email'] on a rejected address that went in with 7.0.3. update_item() was throwing away the WP_Error and applying the address anyway. And a confirmation link followed while logged out lost the confirmation, so the redirect now carries it through redirect_to. I added the unit tests you asked for on the two new functions rather than just their callers, filled in the @SInCE x.x placeholders, and renamed send_user_email_change_confirmation_process() to confirm_user_email_change() since it doesn't send anything. You'd said the naming needed another look. The user_email_confirmed action moved into that function so it fires for both routes instead of only the wp-login.php one. Tests pass on single site and multisite, and PHPStan is clean. If you'd rather pull the commits back onto this branch and close mine, that works for me. Either way I'll keep the discussion on the ticket. |
|
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. |
2 participants
Trac ticket: https://core.trac.wordpress.org/ticket/57413
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.