Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#45492 closed enhancement (wontfix)

Add filter to wp_privacy_generate_personal_data_export_group_html for customizing exported HTML

Reported by: garrett-eclipse's profile garrett-eclipse Owned by: garrett-eclipse's profile garrett-eclipse
Milestone: Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords:
Focuses: Cc:


It would be nice to introduce a filter into wp_privacy_generate_personal_data_export_group_html to enable the customization of HTML markup that will be written to the exported file. This would enable plugins and devs to customized the format of the report.

On use case could be supplementing certain groups with additional description or logo to give more information as to what the export group was for. This would assist in a request generated via Slack;

Attachments (2)

45492.diff (1.2 KB) - added by nickylimjj 5 years ago.
45492.2.diff (1.2 KB) - added by nickylimjj 5 years ago.

Download all attachments as: .zip

Change History (16)

#1 @garrett-eclipse
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to garrett-eclipse
  • Status changed from new to assigned

Related - #45491

#2 @garrett-eclipse
5 years ago

#45491 was marked as a duplicate.

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

5 years ago

5 years ago

#4 @nickylimjj
5 years ago

Created a filter-before-return for requested enhancement. PHPDoc added.

#5 @garrett-eclipse
5 years ago

  • Keywords has-patch needs-testing needs-unit-tests added; needs-patch removed
  • Milestone changed from Future Release to 5.3
  • Status changed from assigned to reviewing

Thanks @nickylimjj I greatly appreciate the patch. This is a great start.

Few minor notes;

  1. Can you update the version to 5.3.0, 5.2 is now in beta so won't accept any more enhancements so I've milestoned this for 5.3
  2. Let's update the description slightly to indicate this is for the personal data export. As well the second line comes from the 'oembed_dataparse' filter isn't fully accurate and I don't feel we need it as the first line should cover us. Let's go with;

Filters the single group HTML for the personal data export report.

  1. Both $group_html and $group_data are params, please update the @return to an @param.

Aside from that this is good to go into testing.

#6 @garrett-eclipse
5 years ago

  • Keywords needs-refresh added

5 years ago

#7 @nickylimjj
5 years ago

  • Keywords needs-refresh removed


#8 @garrett-eclipse
5 years ago

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

Thanks @nickylimjj this looks good and manual tests work nicely. We'll work on some unit tests and move this forward. Cheers

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

5 years ago

#10 @garrett-eclipse
5 years ago

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

As @birgire mentioned in today's Core Privacy Bugscrub there's no need for unit tests as many filters get added to core without it. If there's an existing test, then it might be a good idea to add it in but not required so am dropping that keyword.

@nickylimjj the only minor change flagged in bugscrub was with the $group_html and $group_data params in the docblock should be aligned by adding a single space before the $group_data. Would you mind making that minor change please.

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

5 years ago

#12 @xkon
5 years ago

@nickylimjj thanks for this patch and welcome aboard officially!

It's a really clean patch but unfortunately there is a case that we have to take under serious
consideration here and I'm leaning towards a wontfix for this.

To make an easy example so it's easily understandable you can use the filter from the patch and just add this as an addition on a mu-plugin etc:

	function( $group_html, $group_data ) {
		return '';

The result would just be an empty export page.

Don't get me wrong, but I have to be against this to sleep a little bit better at night as I don't like the idea of freely giving the means to anyone of "easily" removing or altering anything they like from an export that is meant to show intact data.

Even though I do understand the reasoning behind this filter it seems that it can bring way more damage that good if misused.

The original report on slack as mentioned in the OP was

Hi there, regarding the personal data report, I'm finding it a bit impersonal and uninformative as to what each section means to the end user, Is there an option to add a description for each section?

So the actual issue is that each plugin is most likely just adding it's name as a section title to the exporter so simply seeing an "Contact Form" doesn't say much really.

This should be handled by either adding a description field as an "extra" implementation to the exporter -or- by communicating to the Authors that they should be a little bit more descriptive on each section title as the user receiving this export doesn't even know what a plugin is or it's name to identify what the section talks about.

To sum up, I'm in favor of marking this a wontfix and opening a ticket more aligned with the scope of what's actually needed.

#13 @garrett-eclipse
5 years ago

  • Milestone 5.3 deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

Thanks @xkon I appreciate the feedback and completely agree with your points as there is the potential for data scrubbing which should be avoided.

@nickylimjj thank you for the excellent patch and I apologize for leading you astray.

I've reopened #45491 and provided an initial patch to introduce the Group Description for better clarity to assist in the OP issue.

One additional thought was keeping the data safe we could filter the label/description but let's deal with that in the other ticket.

Closing this due to the potential for harm.

#14 @garrett-eclipse
5 years ago

  • Keywords has-patch needs-refresh removed
Note: See TracTickets for help on using tickets.