Make WordPress Core

Opened 2 years ago

Closed 23 months ago

Last modified 22 months ago

#54690 closed feature request (fixed)

Add ability to filter whole notification email in retrieve_password

Reported by: connapptivity's profile connapptivity Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests has-dev-note
Focuses: docs Cc:

Description

There are currently two features missing in the retrieve_password function:

  1. There is no way to filter for the whole notification email with one filter. Only the subject and the message are filterable separately. Including filtering for the whole notification email would follow style of other notification emails like in wp_password_change_notification or in wp_password_change_notification_email.
  2. There is currently no option to add email headers via filters (e.g. for html emails).

Thus, a filter would be very nice (e.g. retrieve_password_notification_email, following naming conventions).

Attachments (2)

54690.diff (1.5 KB) - added by SergeyBiryukov 2 years ago.
54690.2.diff (1.8 KB) - added by costdev 23 months ago.
Removes the no longer necessary $data variable from 54690.diff

Download all attachments as: .zip

Change History (28)

This ticket was mentioned in PR #2082 on WordPress/wordpress-develop by MarloKessler.


2 years ago
#1

Add a filter to retrieve_password to enable filtering the whole retrieve password notification email and following the style of e.g. wp_password_change_notification and wp_new_user_notification. Thus, not two separate filters (retrieve_password_title and retrieve_password_message) must be used and (more importantly) *changing the email headers is now possible too* (e.g. to send HTML emails, which was not possible previously).

I left the retrieve_password_title and retrieve_password_message filters for legacy purposes.

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

#2 @costdev
2 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.0

Hi @connapptivity, welcome to Trac! Thanks for suggesting this new filter and submitting a PR! I have submitted an initial review.

I can appreciate that making changes to individual parts of the email can be frustrating. For this reason, and for consistency with wp_password_change_notification(), I think that adding this all-encompassing filter is a good idea!

For now, I'm going to Milestone this for 6.0.

The retrieve_password() function doesn't appear to have any unit tests. This would be a perfect opportunity to start some. I'm going to add the needs-unit-tests keyword in case anyone on the Test team wants to take a look at this.

You are also welcome to write the unit tests yourself, of course - see the Writing PHP Tests page if you're unfamiliar with this.

Last edited 2 years ago by costdev (previous) (diff)

#3 @audrasjb
2 years ago

  • Keywords needs-refresh added
  • Version trunk deleted

Hello @connapptivity and welcome to WordPress Core Trac.

This is definitely a nice enhancement.
@costdev and I added two change requests in the above PR.

Thank you!

MarloKessler commented on PR #2082:


2 years ago
#4

@costdev, is there anything else to do for this PR?

MarloKessler commented on PR #2082:


2 years ago
#5

@MarloKessler This review is a final pass through the changes in user.php before approving. Awesome work on this PR!

Thank you and thank you as well for your support!

#6 @costdev
2 years ago

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

#7 @audrasjb
2 years ago

  • Owner set to audrasjb
  • Status changed from new to assigned

Thanks all for working on this. Self assigning for commit consideration, as soon as 5.9 is branched.

#8 @audrasjb
2 years ago

  • Keywords needs-dev-note added

We'll need to document this new filter in a dev note for WP 6.0

#9 @audrasjb
2 years ago

Pinging @johnbillion for a quick check before commit, since you are our email notification hooks historian :)

#10 @johnbillion
2 years ago

Concerns discussed on the PR have been addressed, this looks good to go.

#11 @audrasjb
2 years ago

Alright, thank you.

#12 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 52604:

Users: Add new hooks to filter retrieve password emails.

This change introduces two new hooks to help developers to filter retrieve password emails:

  • send_retrieve_password_email can be used to filter whether to send the retrieve password email;
  • retrieve_password_notification_email can be used to filter the contents of the reset password notification email sent to the user.

This changesets also adds unit tests for these new filters.

Props connapptivity, costdev, audrasjb, johnbillion.
Fixes #54690.

#13 @audrasjb
2 years ago

  • Focuses docs added
  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to address a few DocBlocks inconsistencies.

#15 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52605:

Docs: Docblocks consistency fixes after [52604].

Follow-up to [52604].

Fixes #54690.

#16 @SergeyBiryukov
2 years ago

In 52606:

Docs: Further update the send_retrieve_password_email filter documentation for consistency.

Follow-up to [52604], [52605].

See #54690.

#17 @SergeyBiryukov
2 years ago

In 52607:

Users: Revert the variable change in [52606] that caused the tests to fail.

Follow-up to [52604-52606].

Props audrasjb.
See #54690.

@SergeyBiryukov
2 years ago

#18 @SergeyBiryukov
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Great work so far! I would like to suggest a couple of minor improvements :)

  • For consistency with some similar filters like send_password_change_email or send_email_change_email, and for more flexibility, should the new send_retrieve_password_email filter receive the $user_login and $user_data parameters as well?
  • For consistency with the retrieve_password_title and retrieve_password_message filters, could we pass the additional parameters for retrieve_password_notification_email directly, instead of a compacted array? I see this was discussed on the PR, but I think consistency with the existing filters might be beneficial here.

54690.diff implements the suggested changes.

#19 @costdev
2 years ago

@SergeyBiryukov I agree that the new send_retrieve_password_email filter could benefit from receiving $user_login and $user_data. Nice addition!

For retrieve_password_notification_email, my thinking is surrounding readability, especially if we decide to add additional data in future. For example, $site_name.

Taking both consistency and readability into consideration, and taking influence from wpmu_signup_blog_notification_email, what about using multiline to pass the arguments directly?

<?php

$notification_email = apply_filters(
    'retrieve_password_notification_email',
    $defaults,
    $key,
    $user_login,
    $user_data
);

The $data variable's definition should also be removed if we're not using the compacted array.

@costdev
23 months ago

Removes the no longer necessary $data variable from 54690.diff

#20 @costdev
23 months ago

  • Keywords commit added

#22 @peterwilsoncc
23 months ago

Tests are passing for 54690.2.diff and the change makes sense, thanks for catching this before the release @SergeyBiryukov.

@audrasjb Do you have time to do the commit honours?

peterwilsoncc commented on PR #2584:


23 months ago
#23

Closing, this was simply testing a patch.

#24 @audrasjb
23 months ago

  • Keywords assigned-for-commit added

Great, thanks all 👍

#25 @audrasjb
23 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 53178:

Users: Update parameters passed to the new send_retrieve_password_email and retrieve_password_notification_email filters.

For consistency with some similar filters like send_password_change_email or send_email_change_email, and for more flexibility, pass $user_login and $user_data parameters directly to the new send_retrieve_password_email and retrieve_password_notification_email filters.

Follow-up to [52604], [52605], [52606], [52607].

Props SergeyBiryukov, costdev, peterwilsoncc.
Fixes #54690.

#26 @sabernhardt
22 months ago

  • Keywords has-dev-note added; needs-dev-note commit assigned-for-commit removed
Note: See TracTickets for help on using tickets.