#47509 closed enhancement (fixed)
add a filter in wp_user_personal_data_exporter() to make it easier to include additional user meta in a personal data export
Reported by: | pbiron | Owned by: | xkon |
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Privacy | Keywords: | has-patch has-screenshots has-unit-tests needs-docs commit has-dev-note |
Focuses: | Cc: |
Description (last modified by )
wp_user_personal_data_exporter()
has a fixed set of user properties that it includes in a personal data export.
Therefore, plugins that store personal data as user meta have to write their own exporter (and register it with the wp_privacy_personal_data_exporters
).
It would be nice if wp_user_personal_data_exporter()
had a filter that allowed plugins to augment the hard coded list. That way they wouldn't have to write their own exporter.
Attachments (8)
Change History (28)
#3
in reply to:
↑ 1
@
5 years ago
Replying to azaozz:
Also, perhaps the default data should not be configurable/removable by a plugin. Plugins should be able to add data "safely" with no chance to accidentally overwrite other data there.
Correct, that's what I meant by
...allowed plugins to augment the hard coded list
but your clarification makes it much clearer.
#4
@
5 years ago
- Keywords has-patch 2nd-opinion has-screenshots added
- Milestone changed from Awaiting Review to 5.4
- Owner set to xkon
- Status changed from new to assigned
This looks to me like a great idea for any extra data that could be bumped in along with the profile/meta data.
Plugins like BuddyPress, for example, could easily merge their additional fields with this instead of a full new section and it would make more sense IMHO to have everything grouped in 1 dataset.
47509.diff takes a first look at adding a filter there with simple checks to extend the existing default array.
An author (or for anyone that wants to quickly test this) could use something like this:
<?php add_filter( 'wp_privacy_additional_user_data', function( $additional_data, $user_default_data ) { $additional_data = array( array( 'name' => __( 'Data one', 'a-plugin' ), 'value' => 'one', ), array( 'name' => __( 'Data two', 'a-plugin' ), 'value' => 'two', ), array( 'name' => __( 'Data three', 'a-plugin' ), 'value' => 'three', ), array( 'name' => __( 'Data four', 'a-plugin' ), 'value' => 'four', ), ); return $additional_data; }, 10, 2 );
Everything will be placed on export after the default array as seen on 47509_preview.jpg. I've also changed the subtitle from "profile data" to "meta data" as it would fit more the new content of this "group".
I'll mark this for 5.4 in case we could get some extra feedback here or any extra patches to finalize this.
@pbiron any thoughts :) ?
#5
@
5 years ago
@xkon thanx for the patch. I'll try to take a look in the next day or too.
Getting this in 5.4 would be great!
#6
@
5 years ago
Just gave the patch a spin and it seems to work great...and is much easier than registering a separate exporter.
It correctly does not allow altering the data that core exports for a user.
One question though: it does allow a plugin to add items with the same title/name as those exported by core but with a different value...which might be confusing to users. For example:
<?php add_filter( 'wp_privacy_additional_user_data', function( $additional_data, $user_default_data ) { $additional_data = array( array( 'name' => __( 'User ID', 'a-plugin' ), 'value' => '127', ), array( 'name' => __( 'User Login Name', 'a-plugin' ), 'value' => 'anotherrandomuser', ), ); return $additional_data; }, 10, 2 );
I do not think it would be worth stripping out such items from the result of applying the filter...but I just wanted to mention it in case anyone else things it would be a good idea.
#7
follow-up:
↓ 9
@
5 years ago
Ah, thanks for taking a look so fast!
I'm on the same side I've tried "forcing" to not allow any duplicate names, but unfortunately, there's no way of throwing a notice to the devs and stripping them out or any other way that I could think of would produce wrong results, so still, not a good way and I preferred to leave everything as-is.
This is the reason actually that I added the extra param of $user_default_data
at least this way anyone interested can programmatically check it as well (I know that exporting over and over again is tedious work :D) to avoid duplicate titles.
#8
@
5 years ago
For the record, 47509_preview_with_dup_titles.png is what the export would look like with the $additional_data
added by comment:6.
#9
in reply to:
↑ 7
@
5 years ago
Replying to xkon:
I'm on the same side I've tried "forcing" to not allow any duplicate names, but unfortunately, there's no way of throwing a notice to the devs and stripping them out or any other way that I could think of would produce wrong results, so still, not a good way and I preferred to leave everything as-is.
Stripping out the duplicates is not hard. There might be more "elegant" ways, but the following works just fine:
<?php $reserved_names = wp_list_pluck( $user_data_to_export, 'name' ); $extra_data = apply_filters( 'wp_privacy_additional_user_data', array(), $user_data_to_export ); #extra_data = array_filter( $extra_data, function( $item ) use ( $reserved_names ) { return ! in_array( $item['name'], $reserved_names ); } );
This is the reason actually that I added the extra param of
$user_default_data
at least this way anyone interested can programmatically check it as well (I know that exporting over and over again is tedious work :D) to avoid duplicate titles.
I figured that's why you passed that parameter to the filter...it's what gave me the idea to see what happened if I returned duplicates :-)
With that in mind, it might be a good idea to add something to the DocBlock for the filter that tells plugin authors they should not return items with any of the names from $user_data_to_export
. And specifically, that any such items will not change what is exported by core and that the duplicates may confuse users.
This ticket was mentioned in Slack in #core-privacy by pbiron. View the logs.
5 years ago
#11
@
5 years ago
47509.2.diff updates 47509.diff, with the following changes:
- removes any item in the array returned by the filter that uses a
name
that core is already exporting. - improves the DocBlock for the filter
- passes the user whose data is being exported to the filter (would be kind of hard for plugins to use the filter without that :-)
- passes an array of
$reserved_names
, instead of$user_data_to_export
to the filter. This makes it easier to document that any item returned that uses one of the$reserved_names
will not be exported. - calls
_doing_it_wrong()
if any items were removed.
I'm still not sure whether number 5 is the correct thing to do. I like the idea of it, but there don't seem to any other cases in core where _doing_it_wrong()
is called when a filter returns results that aren't correct. I'll leave it a committer to decide whether that is a correct usage of _doing_it_wrong()
.
#12
@
5 years ago
- Keywords has-unit-tests needs-dev-note needs-docs added
Thanks for the update @pbiron! It works as expected for me.
47509.3.diff also adds unit tests for the filter.
I'm marking this for commit as I do think that _doing_it_wrong
gives extra feedback to help developers as well, IMO it's better than silently removing the data and nobody noticing, but we'll see :-).
#14
@
5 years ago
45709.4.diff improves the unit test:
- improves the check for the additional data added by the callback
- hooks a 2nd callback that should generate the
_doing_it_wrong()
and checks that dupname
is correctly stripped in that case
@
5 years ago
Minor refresh to rename the filter wp_privacy_additional_user_profile_data
to match the group this is paired with
#15
@
5 years ago
- Keywords 2nd-opinion removed
Thanks @pbiron & @xkon for pushing this along it's looking really good and tests well both manually and the unit tests.
I made some minor changes in 47509.5.diff as follows;
- Changed docblock
@param string[]
to@param array
- Sentence cased the comments.
- Updated the filter name to
wp_privacy_additional_user_profile_data
as this includes the data into the User's profile data group. Other meta such as Session Tokens and Community Events Location information will get dedicated groupings so wanted to be explicit this is for user_profile_data and not just user_data in general.
Marking for commit, feel free to do some additional testing with the changed filter name.
This ticket was mentioned in Slack in #core-privacy by pbiron. View the logs.
5 years ago
#18
@
5 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Quick look at 47509.5.diff, in_array( $item['name'], $reserved_names );
should be a strict comparison (true
at the end), both are strings. Not a big deal, but...
This was discussed a bit in Slack. It makes sense for plugins to be able to add more/other data they have stored in user meta, and perhaps in other places as long as the data is fast to get.
Also, perhaps the default data should not be configurable/removable by a plugin. Plugins should be able to add data "safely" with no chance to accidentally overwrite other data there.