WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 5 days ago

#44314 new defect (bug)

`user_confirmed_action_email_content` filter run on two different strings

Reported by: desrosj Owned by:
Milestone: 4.9.7 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: needs-patch
Focuses: Cc:

Description

The user_confirmed_action_email_content filter is run in two different functions that have two completely different strings passed.

In _wp_privacy_send_erasure_fulfillment_notification(), the filter is applied to the body content of the email informing the user that their data erasure request was successfully fulfilled by an administrator.

In _wp_privacy_send_request_confirmation_notification(), the filter is applied to the body content of the email sent to an administrator when the user confirms a request to perform an action on their data.

The filter in _wp_privacy_send_erasure_fulfillment_notification() should be renamed to something more appropriate.

A scan of the plugin directory yields 0 results for occurrence of user_confirmed_action_email_content, and a quick search in GitHub shows only forks of WordPress/repositories that have WordPress Core inside them contain this filter, so changing it now before developers start filtering the email content should be ok.

Attachments (2)

44314.diff (1022 bytes) - added by desrosj 3 weeks ago.
44314.2.diff (2.6 KB) - added by birgire 8 days ago.

Download all attachments as: .zip

Change History (8)

@desrosj
3 weeks ago

#1 @desrosj
8 days ago

#44380 was marked as a duplicate.

#2 @garrett-eclipse
8 days ago

This looks great @desrosj

The only edge case I wonder about as you confirmed that no plugins use that filter is any users who've custom coded using that filter and expect their fulfill email to update. Should we rename both instances so they're new and then with the original string setup a doing it wrong filter that indicates they should use one of either the others so as to avoid customizations that some users have done and expect it on the fulfill or both and now it's not doing it there. I'm probably overthinking but just wanted to put that out there.

Cheers

#3 @birgire
8 days ago

44314.2.diff takes the same approach as #44341, using the apply_filters_deprecated(), available since 4.6:

When a filter hook is deprecated, the apply_filters() call is replaced with apply_filters_deprecated(), which triggers a deprecation notice and then fires the original filter hook.

@desrosj and @garrett-eclipse, what's your take on this approach here?

@birgire
8 days ago

#4 @garrett-eclipse
6 days ago

Thanks @birgire appreciated, I'll leave that to @desrosj as it may be overkill since it was so recently introduced but is a good approach is we feel that extra step is needed. Cheers

#5 @desrosj
6 days ago

@birgire this would probably be the safest approach. I am not sure about the proper way to update the inline documentation though. It doesn't feel right to have the full docblock for both the deprecated and replacement filter.

There are only 2 other uses of apply_filters_deprecated() currently in core, so I am not sure the right way to update the documentation. @DrewAPicture maybe you have some guidance here?

#6 @DrewAPicture
5 days ago

  • Keywords needs-patch added; has-patch removed

I don't think we should break back-compat/deprecate here particularly because this was introduced in a point release.

I think the best option here is to probably introduce a third $context parameter so that callbacks can differentiate between the different ways the filter is being evaluated.

Last edited 5 days ago by DrewAPicture (previous) (diff)
Note: See TracTickets for help on using tickets.