Make WordPress Core

Opened 6 years ago

Closed 3 years ago

#44314 closed defect (bug) (fixed)

`user_confirmed_action_email_content` filter run on two different strings

Reported by: desrosj's profile desrosj Owned by: garrett-eclipse's profile garrett-eclipse
Milestone: 5.8 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests needs-dev-note early commit dev-reviewed
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 (13)

44314.diff (1022 bytes) - added by desrosj 6 years ago.
44314.2.diff (2.6 KB) - added by birgire 6 years ago.
44314.3.diff (4.5 KB) - added by garrett-eclipse 5 years ago.
Updated patch for deprecating the user_confirmed_action_email_content for erasure fulfillment emails
44314.4.diff (4.6 KB) - added by audrasjb 4 years ago.
Patch refresh for WP 5.5
44314.5.diff (21.3 KB) - added by garrett-eclipse 4 years ago.
Refresh to deprecated user_confirmed_action_email_content, user_erasure_complete_email_subject, user_confirmed_action_email_content and user_erasure_complete_email_headers
44314-email-log.pdf (50.2 KB) - added by hellofromTonya 4 years ago.
Captured emails (exported from Email Log plugin) of testing results
44314.6.diff (20.0 KB) - added by xkon 4 years ago.
refresh
44314.7.diff (20.0 KB) - added by antpb 4 years ago.
44314.7
44314.8.diff (19.6 KB) - added by audrasjb 4 years ago.
patch refreshed according to Peter's comments
44314.cc.diff (19.6 KB) - added by peterwilsoncc 4 years ago.
44314.cc.2.diff (19.9 KB) - added by coffee2code 4 years ago.
Update to 44314.cc.diff with changes I mentioned in comments 59 and 60.
44314.9.diff (21.0 KB) - added by peterwilsoncc 4 years ago.
44314.10.diff (8.0 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (94)

@desrosj
6 years ago

#1 @desrosj
6 years ago

#44380 was marked as a duplicate.

#2 @garrett-eclipse
6 years 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
6 years 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
6 years ago

#4 @garrett-eclipse
6 years 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 years 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
6 years 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 6 years ago by DrewAPicture (previous) (diff)

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


6 years ago

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

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

#10 @pbiron
6 years ago

  • Keywords needs-patch removed

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


6 years ago

#12 @desrosj
6 years ago

  • Keywords has-patch added

4.9.8 is in RC. Moving to 4.9.9.

#13 @desrosj
6 years ago

  • Milestone changed from 4.9.8 to 4.9.9

#14 @birgire
6 years ago

#44843 was marked as a duplicate.

#15 follow-up: @desrosj
6 years 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.


6 years ago

#17 @pento
6 years ago

  • Milestone changed from 4.9.9 to Future Release

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


6 years ago

#19 @coffee2code
6 years 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
6 years ago

  • Keywords needs-refresh added

@garrett-eclipse
5 years ago

Updated patch for deprecating the user_confirmed_action_email_content for erasure fulfillment emails

#21 @garrett-eclipse
5 years 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.


5 years ago

#23 @TZ Media
5 years 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.


5 years ago

#25 @garrett-eclipse
5 years ago

#47801 was marked as a duplicate.

#26 @SergeyBiryukov
5 years ago

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

#27 in reply to: ↑ 15 ; follow-up: @SergeyBiryukov
5 years 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
5 years 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 5 years ago by SergeyBiryukov (previous) (diff)

#29 @garrett-eclipse
5 years 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.

#30 @garrett-eclipse
5 years ago

  • Keywords early added
  • Milestone changed from 5.4 to 5.5

I didn't have a chance in the 5.4 cycle to tackle this so am bumping to 5.5.

#31 @garrett-eclipse
5 years ago

Note: In WordPress 5.4 there were several filters added to control the email headers for privacy-related emails in [47279] / #44501.

  • wp_privacy_personal_data_email_headers
  • user_request_confirmed_email_headers
  • user_erasure_complete_email_headers
  • user_request_action_email_headers

These should be taken into account when we address this in 5.5

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


4 years ago

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


4 years ago

#34 @johnbillion
4 years ago

List of plugins in the w.org directory that use this filter, for informational purposes: https://wpdirectory.net/search/01E6M1WKQEZ138F0WV077RSHM5

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


4 years ago

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


4 years ago

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


4 years ago

@audrasjb
4 years ago

Patch refresh for WP 5.5

#38 @davidbaumwald
4 years ago

  • Milestone changed from 5.5 to Future Release

With 5.5 Beta 1 approaching, the time for early tickets has now passed, and this ticket is being moved to Future Release. If any maintainer or committer feels this can be quickly resolved for 5.5 or wishes to assume ownership of this ticket during a specific cycle, feel free to update the milestone accordingly.

@garrett-eclipse
4 years ago

Refresh to deprecated user_confirmed_action_email_content, user_erasure_complete_email_subject, user_confirmed_action_email_content and user_erasure_complete_email_headers

#39 @garrett-eclipse
4 years ago

  • Keywords needs-testing added; needs-refresh removed
  • Status changed from reviewing to accepted

Sorry this ended up being bumped a bit, let's try to get it into early 5.6 here before that ship sails again.

I've uploaded 44314.5.diff to account for the direction @SergeyBiryukov provided along with accounting for new filters introduced since his comment in 5.3 and 5.4.

Full Overview of filter changes;

  • _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)
    • user_request_confirmed_email_headers
  • _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)
    • user_erasure_complete_email_headers (2)
    • user_erasure_fulfillment_email_headers (new)
  • wp_send_user_request()
    • user_request_action_email_subject
    • user_request_action_email_content
    • user_request_action_email_headers

NOTE: There is no _to filter here as the email is always expected to be sent to the $request->email.

  • wp_privacy_send_personal_data_export_email()
    • wp_privacy_export_expiration
    • wp_privacy_personal_data_email_to
    • wp_privacy_personal_data_email_subject
    • wp_privacy_personal_data_email_content
    • wp_privacy_personal_data_email_headers

Note: These are the only ones with the wp_ prefix but they are mostly consistent aside from the export expiration one so I've left alone for now.

Once this is committed the Privacy article here will need updating;
https://developer.wordpress.org/plugins/privacy/privacy-related-options-hooks-and-capabilities/

The following plugins are affected by this change;

All feedback/testing welcome.

#40 @garrett-eclipse
4 years ago

  • Milestone changed from Future Release to 5.6

Oops missed milestoning this, set for 5.6

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


4 years ago

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


4 years ago

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


4 years ago

#44 @hellofromTonya
4 years ago

📣 Call for testers 📣

Notes from today's Privacy scrub.

This ticket is blocked from commit as it needs-testing. It risks getting punted from 5.6 as Beta 1 is less than a week away.

Testing focus per Garrett:

confirm I didn’t mistype any of the filter names

He notes that testing is:

a bit bigger yes, will need someone to write php using the filters to confirm they work as expected or show the dep flag as expected

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


4 years ago

#46 @hellofromTonya
4 years ago

  • Keywords needs-testing removed

Testing results

Used the following filtered code:

<?php
// _wp_privacy_send_request_confirmation_notification
add_filter( 'user_confirmed_action_email_content', function( $content ) { return $content . ' DEPRECATED'; } );
add_filter( 'user_request_confirmed_email_content', function( $content ) { return $content . ' FILTERED'; } );

// _wp_privacy_send_erasure_fulfillment_notification
add_filter( 'user_erasure_complete_email_subject', function( $subject ) { return $subject . ' DEPRECATED'; } );
add_filter( 'user_erasure_fulfillment_email_subject', function( $subject ) { return $subject . ' FILTERED'; } );
add_filter( 'user_erasure_fulfillment_email_content', function( $content ) { return $content . ' FILTERED'; } );
add_filter( 'user_erasure_complete_email_headers', function( $headers ) { return $headers; } );
add_filter( 'user_erasure_fulfillment_email_headers', function( $headers ) { return $headers; } );

The deprecated filters generated the following results in the src/wp-content/debug.log file:

[24-Oct-2020 21:32:01 UTC] PHP Deprecated:  user_confirmed_action_email_content is <strong>deprecated</strong> since version 5.6.0! Use user_request_confirmed_email_content instead. in /var/www/src/wp-includes/functions.php on line 5211
[24-Oct-2020 21:36:42 UTC] PHP Deprecated:  user_erasure_complete_email_subject is <strong>deprecated</strong> since version 5.6.0! Use user_erasure_fulfillment_email_subject instead. in /var/www/src/wp-includes/functions.php on line 5211
[24-Oct-2020 21:36:42 UTC] PHP Deprecated:  user_confirmed_action_email_content is <strong>deprecated</strong> since version 5.6.0! Use user_erasure_fulfillment_email_content instead. in /var/www/src/wp-includes/functions.php on line 5211
[24-Oct-2020 21:36:42 UTC] PHP Deprecated:  user_erasure_complete_email_headers is <strong>deprecated</strong> since version 5.6.0! Use user_erasure_fulfillment_email_headers instead. in /var/www/src/wp-includes/functions.php on line 5211
[24-Oct-2020 21:37:15 UTC] PHP Deprecated:  user_confirmed_action_email_content is <strong>deprecated</strong> since version 5.6.0! Use user_request_confirmed_email_content instead. in /var/www/src/wp-includes/functions.php on line 5211
[24-Oct-2020 21:39:30 UTC] PHP Deprecated:  user_erasure_complete_email_subject is <strong>deprecated</strong> since version 5.6.0! Use user_erasure_fulfillment_email_subject instead. in /var/www/src/wp-includes/functions.php on line 5211
[24-Oct-2020 21:39:30 UTC] PHP Deprecated:  user_confirmed_action_email_content is <strong>deprecated</strong> since version 5.6.0! Use user_erasure_fulfillment_email_content instead. in /var/www/src/wp-includes/functions.php on line 5211
[24-Oct-2020 21:39:30 UTC] PHP Deprecated:  user_erasure_complete_email_headers is <strong>deprecated</strong> since version 5.6.0! Use user_erasure_fulfillment_email_headers instead. in /var/www/src/wp-includes/functions.php on line 5211

The emails received: See PDF attachment.

@hellofromTonya
4 years ago

Captured emails (exported from Email Log plugin) of testing results

#47 @garrett-eclipse
4 years ago

  • Keywords commit added; early removed

Thank you so much for the thorough testing and confirmations.

@desrosj / @SergeyBiryukov is this something we can still land in 5.7?

Version 0, edited 4 years ago by garrett-eclipse (next)

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


4 years ago

#49 @helen
4 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.6 to 5.7

Thanks to everyone for the testing work here. I am going to punt from 5.6, it's too close to enhancement territory since nobody is using these filters still and we have quite a bit of other things to work through. Patch seems to need a mild refresh for some unrelated changes as well :)

@xkon
4 years ago

refresh

#50 @xkon
4 years ago

  • Keywords needs-refresh removed

44314.6.diff is a refresh of previous 44314.5.diff to cleanly apply. I've also adjusted the @since mentions.

@antpb
4 years ago

44314.7

#51 @antpb
4 years ago

Fixed some spacing in the doc blocks and an @since.

This could use a final review if you are able @SergeyBiryukov :) Thanks!

#52 @peterwilsoncc
4 years ago

  • Keywords needs-refresh added; commit removed

@antpb

Upon a visual review of 44314.7.diff :

  • deprecated filters are showing 5.6 rather than 5.7.
  • CS Nit: Param docblocks not aligned for user_request_action_email_content
  • Typically the deprecated filters don't have an @ignore tag (it seems the description is often cut back to use the [filter] filter instead.

Otherwise it looks good.

In 5.6 this was tagged early and the commit keyword has been sitting on it throughout the 5.7 milestone. Is it time to move this to 5.8? If so, my first bullet point is out of date ;)

I've removed the commit keyword and added needs refresh, even though the changes are minor.

@audrasjb
4 years ago

patch refreshed according to Peter's comments

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


4 years ago

#54 @audrasjb
4 years ago

  • Keywords needs-refresh removed

#55 @peterwilsoncc
4 years ago

I'm not sure why but 44314.8.diff was showing as corrupt: 44314.cc.diff is a carbon copy.

#56 @peterwilsoncc
4 years ago

These changes look good but a lot for the week before RC so I am inclined not to for this release and suggest they be merged shortly after the branch is forked.

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


4 years ago

#58 @hellofromTonya
4 years ago

  • Keywords early added
  • Milestone changed from 5.7 to 5.8

I agree with Peter:

These changes look good but a lot for the week before RC so I am inclined not to for this release and suggest they be merged shortly after the branch is forked.

With RC1 tomorrow and the amount of changes, the patch for this ticket will benefit from early and a full beta cycle. Moving this ticket to 5.8 and marking for early.

#59 @coffee2code
4 years ago

The latest patch by @peterwilsoncc overall looks good, except for the one point I brought up in my previous comment (which, admittedly, might not have been clear enough).

Basically, DevHub can only know about one instance of a hook name.

However, there are two instances of the 'user_confirmed_action_email_content' filter. If you check DevHub currently, you may note that the docs for the hook were parsed from the version of the hook defined in _wp_privacy_send_erasure_fulfillment_notification(). That filter also appears in _wp_privacy_send_request_confirmation_notification(), but doesn't appear in DevHub even though it is documented differently.

The aforementioned patch properly deprecates the duplicate hooks as if they were differently-named. But since you can't rely on which version of the hook docs will appear on DevHub for 'user_confirmed_action_email_content', the deprecation docs need to cover both cases.

Also consider things from a developer's point of view: If someone had previously hooked 'user_confirmed_action_email_content', that callback will get invoked in either of 2 functions. WP can't accurately report to them whether they should be using 'user_erasure_fulfillment_email_content' or 'user_request_confirmed_email_content' because it doesn't know which was really intended. The error message should report both as possible alternatives to avoid recommending the wrong one.

So there are two main aspects of the patch that need adjusting to convey the hook has two possible alternatives:

1. Address how deprecation is conveyed via docs in DevHub

Since only one version of the hook docs will be presented in DevHub, they both need to reference the possibility of two recommended alternatives. Or, one hook can get a @ignore added so that only the other instance of the hook needs to reference both hooks.

The @deprecated notice should reference both alternatives:

@deprecated 5.7.0 Use {@see 'user_erasure_fulfillment_email_content'} instead. For user request confirmation email content use {@see 'user_request_confirmed_email_content'} instead.

(This is for the instance of the hook in _wp_privacy_send_erasure_fulfillment_notification(). Something similar would need to be done in _wp_privacy_send_request_confirmation_notification(), unless that instance got an @ignore instead. But I think ignoring one would prevent it from appearing under the function's Used By section, so probably best not to do.)

2. Address how deprecation is conveyed via error message

Here is an example of how the aforementioned proposed patch formally deprecates an instance of the hook:

$content = apply_filters_deprecated( 'user_confirmed_action_email_content', array( $content, $email_data ), '5.7.0', 'user_erasure_fulfillment_email_content' );

There are two possible approaches to clarifying the recommended alternatives:

  1. Overload the $replacement arg of the apply_filters_deprecated() call (the 4th arg) with both names, either as "user_erasure_fulfillment_email_content|user_request_confirmed_email_content" or "user_erasure_fulfillment_email_content or user_request_confirmed_email_content"

    e.g. $content = apply_filters_deprecated( 'user_confirmed_action_email_content', array( $content, $email_data ), '5.7.0', 'user_erasure_fulfillment_email_content or user_erasure_fulfillment_email_content' );

    Resulting error log message to dev:
    user_confirmed_action_email_content is <strong>deprecated</strong> since version 5.7.0! Use user_erasure_fulfillment_email_content or user_request_confirmed_email_content instead.
  1. Add the other possible replacement as the $message arg of the apply_filters_deprecated() call (a new 5th arg):

    e.g. $content = apply_filters_deprecated( 'user_confirmed_action_email_content', array( $content, $email_data ), '5.7.0', 'user_erasure_fulfillment_email_content', __( "If this was intended to hook within _wp_privacy_send_erasure_fulfillment_notification(), then use user_erasure_fulfillment_email_content instead." ) );

    Resulting error log message to dev:
    user_confirmed_action_email_content is <strong>deprecated</strong> since version 5.7.0! Use user_erasure_fulfillment_email_content instead. If this was intended to hook within _wp_privacy_send_request_confirmation_notification(), then use user_request_confirmed_email_content instead.

Patch forthcoming that addresses 1 by having each deprecated instance of the hook refer to the other (without any use of @ignore) and addresses 2 using the approach in 2a (using " or " instead of "|").

#60 @coffee2code
4 years ago

One minor consideration regarding 44314.cc.diff, with respect to the formatting of the @deprecated lines. Including the word "filter" after the @see

e.g. Use {@see 'user_request_confirmed_email_content'} filter instead.

is rarely done in core. There appear to be only 3 instances of it. Omitting the word

e.g. Use {@see 'user_request_confirmed_email_content'} instead.

has 34 existing similar formatting uses, so the convention would be to omit it.

My forthcoming patch will account for this.

@coffee2code
4 years ago

Update to 44314.cc.diff with changes I mentioned in comments 59 and 60.

#61 @peterwilsoncc
4 years ago

Thanks so much for your help with this @coffee2code!

44314.9.diff has a very minor change: it internationalizes the word 'or' in the deprecation alternatives

<?php
sprintf(
        /* translators: 1 & 2: Deprecation replacement options */
        __( '%1$s or %2$s' ),
        'user_request_confirmed_email_content',
        'user_erasure_fulfillment_email_content'
)

#62 @coffee2code
4 years ago

Looks good @peterwilsoncc!

#63 @lukecarbis
4 years ago

@peterwilsoncc Is this ready to be marked for commit?

#64 @peterwilsoncc
4 years ago

  • Keywords commit added

@lukecarbis it is and marking as such.

This is currently on my list of things to ignore until 5.7.1 is released but if anyone else wants to put it in, feel free.

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


4 years ago

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


3 years ago

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


3 years ago

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


3 years ago

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


3 years ago

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


3 years ago

This ticket was mentioned in PR #1355 on WordPress/wordpress-develop by hellofromtonya.


3 years ago
#71

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

  • Applies 44314.9.diff
  • Updates version from 5.7.0 to 5.8.0

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


3 years ago

#73 @antpb
3 years ago

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

In 51129:

Privacy: Improve naming of user confimed action email filters.

The filters user_confirmed_action_email_content, user_erasure_complete_email_subject, and user_erasure_complete_email_headers have been deprecated.

They have been replaced with user_erasure_fulfillment_email_content, user_erasure_fulfillment_email_subject, and user_erasure_fulfillment_email_headers.

Props desrosj, garrett-eclipse, birgire, DrewAPicture, lifeforceinst, ocean90, pbiron, pento, coffee2code, TZ-Media, SergeyBiryukov, johnbillion, audrasjb, davidbaumwald, hellofromTonya, helen, xkon, antpb, peterwilsoncc, lukecarbis.
Fixes #44314.

#75 @johnbillion
3 years ago

In 51353:

Docs: Correct the @since tag for the user_erasure_fulfillment_email_headers filter.

See #44314, #53461

#76 @desrosj
3 years ago

In 51354:

Docs: Correct the @since tag for the user_erasure_fulfillment_email_headers filter.

Props johnbillion.
See #44314, #53461.

#77 @SergeyBiryukov
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for some minor cleanup:

  • There are some typos in filter descriptions: "an user", "when a their data".
  • Per the documentation standards, DocBlocks should preferably wrap to the next line after 80 characters or so, and should not extend beyond a total of 120 characters wide.
  • The deprecation message itself clearly states the suggested replacement, as seen on dbx_post_advanced hook page, for example. I don't see a reason to duplicate this in the hook description, and it looks like we don't generally do that for other deprecated hooks. I think adding this to the @deprecated tag is sufficient.
  • I think the unneccessary indent adjument, along with the phpcs:ignore comment, can be removed. This is not really consistent with other similar emails in core on even in the same file. If someone feels strongly about removing extra tabs from the first lines here, let's do that consistently for all emails in a new ticket.

See 44314.10.diff.

#78 @desrosj
3 years ago

  • Keywords dev-reviewed added

@SergeyBiryukov 44314.10.diff looks good for commit and backport to me!

#79 @desrosj
3 years ago

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

In 51410:

Docs: Various documentation fixes following [51129].

  • Typo corrections in filter descriptions.
  • DocBlocks are now now wrapped to the next line after 80 characters, and not extending beyond 120 total characters wide.
  • Remove unnecessary repeated references to the suggested replacement hooks.
  • Adjustments to the indentation for consistency with other emails in Core, allowing the phpcs:ignore comment to be removed.

Props SergeyBiryukov.
Fixes #44314.

#80 @desrosj
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to backport.

#81 @desrosj
3 years ago

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

In 51411:

Docs: Various documentation fixes following [51129].

  • Typo corrections in filter descriptions.
  • DocBlocks are now now wrapped to the next line after 80 characters, and not extending beyond 120 total characters wide.
  • Remove unnecessary repeated references to the suggested replacement hooks.
  • Adjustments to the indentation for consistency with other emails in Core, allowing the phpcs:ignore comment to be removed.

Props SergeyBiryukov.
Merges [51410] to the 5.8 branch.
Fixes #44314.

Note: See TracTickets for help on using tickets.