WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 2 weeks ago

#44314 reviewing defect (bug)

`user_confirmed_action_email_content` filter run on two different strings

Reported by: desrosj Owned by: garrett-eclipse
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests needs-refresh needs-dev-note
Focuses: Cc:
PR Number:

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 (3)

44314.diff (1022 bytes) - added by desrosj 17 months ago.
44314.2.diff (2.6 KB) - added by birgire 16 months ago.
44314.3.diff (4.5 KB) - added by garrett-eclipse 6 months ago.
Updated patch for deprecating the user_confirmed_action_email_content for erasure fulfillment emails

Download all attachments as: .zip

Change History (32)

@desrosj
17 months ago

#1 @desrosj
16 months ago

#44380 was marked as a duplicate.

#2 @garrett-eclipse
16 months 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
16 months 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
16 months ago

#4 @garrett-eclipse
16 months 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
16 months 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
16 months ago

  • Keywords needs-patch added; has-patch removed

I don't think we should break back-compat 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.

Version 0, edited 16 months ago by DrewAPicture (next)

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


16 months ago

#8 @lifeforceinst
16 months ago

While 'user_confirmed_action_email_content' is called by both _wp_privacy_send_erasure_fulfillment_notification() and _wp_privacy_send_request_confirmation_notification() there are two items passed being: $email_text, $email_data.

$email_data is an array, which contains a number of pieces of information
One option could be to add to the array of $email_data an option called $request_type of "request_export" or "request_erasure".

In this way you would not need to create new hooks, depreciate the existing hook, but just have an additional parameter which people can use to determine which type of email is being generated.

While on this topic of $email_data it could also be useful to pass an additional parameter $message_sender, and process that appropriately so that there is any easy way to hook in to customise all aspects of the email including: sender, title, recipient and content.

#9 @ocean90
16 months ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

#10 @pbiron
15 months ago

  • Keywords needs-patch removed

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


15 months ago

#12 @desrosj
15 months ago

  • Keywords has-patch added

4.9.8 is in RC. Moving to 4.9.9.

#13 @desrosj
15 months ago

  • Milestone changed from 4.9.8 to 4.9.9

#14 @birgire
14 months ago

#44843 was marked as a duplicate.

#15 follow-up: @desrosj
14 months ago

The only problem with not deprecating one or both calls to this filter is that the developer is never informed that they are (at no fault of their own) using it incorrectly. Passing additional information to the filter corrects the mistake somewhat silently. A deprecated call will inform developers of the bug and give a clear path to proceed.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


13 months ago

#17 @pento
12 months ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


12 months ago

#19 @coffee2code
11 months ago

As far as the debate around either renaming one (or both) of the filters or adding an additional context argument goes, I believe the uses of the filters are different enough, and they are documented differently enough, that a filter should be renamed and the original version deprecated.

The issue of filter documentation deserves some consideration in the matter. The convention (if not requirement) for hook names has been that a hook can be reused in multiple places if it is applied identically and can use the same documentation. The latter is important for the Code Reference because only one canonical documentation of a hook is expected; duplicate instances of the hook should be documented with a special comment referencing the canonical documentation.

That can't happen for the instances of this hook due to:

  • The hooks' purposes ("Filters the body of the user request confirmation email." vs "Filters the body of the data erasure fulfillment notification.")
  • The unique placeholder strings supported by each context (only 2 of 6 overall placeholders overlap)
  • The differing elements supplied in the second argument ($email_data) passed to hooking functions

With so much different (the documentation, the arguments, and how the text is handled immediately after filtering), the hooks deserve to be uniquely named. Adding a context argument should be a consideration when only the context of the hook being run is different and all else being the same (or trivially so), not when the type of data passed along differs and the handling of the filtered text differs.

Worth noting: If only one hook is being renamed (and the old name deprecated), the deprecated version (if retaining its docblock) should also have @ignore added to its docblock to ensure only 1 docblock for user_confirmed_action_email_content gets parsed. If both instances get deprecated, then only one needs the @ignore.

#20 @garrett-eclipse
9 months ago

  • Keywords needs-refresh added

@garrett-eclipse
6 months ago

Updated patch for deprecating the user_confirmed_action_email_content for erasure fulfillment emails

#21 @garrett-eclipse
6 months ago

  • Keywords needs-testing has-unit-tests added
  • Milestone changed from Future Release to 5.3

Thanks @coffee2code and everyone for the work on this.

After reviewing the filters and documentation issues they present I've supplied a refresh (44314.3.diff) on @birgire's original patch which passes the $content along as well as updates the corresponding unit test to use the appropriate filter name.

I've also added an @ignore and included a note on the filter that will be staying in order for the reference to be picked up in doc parsers.

To illustrate the existing documentation issues below are some references which show how the filter has been parsed in multiple places with different descriptions. Also note none provide any context about how to differentiate the filters.
https://developer.wordpress.org/reference/hooks/user_confirmed_action_email_content/
https://wpseek.com/hook/user_confirmed_action_email_content/
https://wp-kama.com/hook/user_confirmed_action_email_content
https://developer.wordpress.org/plugins/privacy/privacy-related-options-hooks-and-capabilities/
*In some cases the docs show the fulfillment info and others the confirmed export.

Milestoning this to 5.3 in hopes to address it before it gestates too long.

This ticket was mentioned in Slack in #core-privacy by postphotos. View the logs.


4 months ago

#23 @TZ Media
4 months ago

  • Keywords needs-refresh needs-testing removed

For the current patch, I can't find any issues. Also the unit tests are running correctly for me. The documentation issues should also be fixed. I can't see how this might break something that was working before.

So in my opinion the current patch is ready for commit.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


3 months ago

#25 @garrett-eclipse
3 months ago

#47801 was marked as a duplicate.

#26 @SergeyBiryukov
3 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#27 in reply to: ↑ 15 ; follow-up: @SergeyBiryukov
2 months ago

Replying to desrosj:

The only problem with not deprecating one or both calls to this filter is that the developer is never informed that they are (at no fault of their own) using it incorrectly. Passing additional information to the filter corrects the mistake somewhat silently. A deprecated call will inform developers of the bug and give a clear path to proceed.

I might be missing something, but:

  • If a plugin expects the user_confirmed_action_email_content filter to only fire in _wp_privacy_send_request_confirmation_notification() and not in _wp_privacy_send_erasure_fulfillment_notification(), seems like they are "doing it right" and don't need to change anything?
  • If that's the case, how would they proceed to fix the deprecated notice?

Looking at the function and hook names, they are somewhat inconsistent:

  • _wp_privacy_send_request_confirmation_notification()
    • user_request_confirmed_email_to
    • user_request_confirmed_email_subject
    • user_confirmed_action_email_content
  • _wp_privacy_send_erasure_fulfillment_notification()
    • user_erasure_fulfillment_email_to
    • user_erasure_complete_email_subject
    • user_confirmed_action_email_content (2)
  • wp_send_user_request()
    • user_request_action_email_content
    • user_request_action_email_subject

Could we bring more consistency here?

  • _wp_privacy_send_request_confirmation_notification()
    • user_request_confirmed_email_to
    • user_request_confirmed_email_subject
    • user_confirmed_action_email_content
    • user_request_confirmed_email_content (new)
  • _wp_privacy_send_erasure_fulfillment_notification()
    • user_erasure_fulfillment_email_to
    • user_erasure_complete_email_subject
    • user_erasure_fulfillment_email_subject (new)
    • user_confirmed_action_email_content (2)
    • user_erasure_fulfillment_email_content (new)

Proposal:

  • Deprecate both instances of user_confirmed_action_email_content.
  • Introduce user_request_confirmed_email_content and user_erasure_fulfillment_email_content.
  • Deprecate user_erasure_complete_email_subject in favor of user_erasure_fulfillment_email_subject for consistent naming.

#28 in reply to: ↑ 27 @SergeyBiryukov
2 months ago

Replying to SergeyBiryukov:

  • _wp_privacy_send_request_confirmation_notification()
    • user_request_confirmed_email_to
    • user_request_confirmed_email_subject
    • user_confirmed_action_email_content
    • user_request_confirmed_email_content (new)

Ideally, these would be user_request_confirmation_email_* (with a noun instead of a verb, for consistency with user_erasure_fulfillment_*, but that ship has sailed :)

Last edited 2 months ago by SergeyBiryukov (previous) (diff)

#29 @garrett-eclipse
2 weeks ago

  • Keywords needs-refresh needs-dev-note added
  • Milestone changed from 5.3 to 5.4
  • Owner changed from SergeyBiryukov to garrett-eclipse

Thanks @SergeyBiryukov I appreciate the thorough feedback and agree with your proposal.

While I would love to resolve the duplication sooner than later I would be more comfortable landing a change involving deprecations early and not in beta2. I've moved this to 5.4 early and taken ownership of the refresh and accompaniment of a dev-note.

I also wanted to flag a related #46303 which was committed in ChangeSet#46265 that introduced wp_privacy_personal_data_email_to and wp_privacy_personal_data_email_subject to the wp_privacy_send_personal_data_export_email. *These will need to be taken into account in the proposal here.

Note: See TracTickets for help on using tickets.