Organize privacy functions into logical files and classes

[iandunn]

Many new functions and classes are being created in 4.9.6 for the privacy features, but they can only be added in existing files, because it's a point release (and they have to be in the next point release, because GDPR).

That results in a bit of a mess, since functions are tacked on to unrelated files, or files that already have thousands of lines of code. The codebase would be much more maintainable if we can migrate logical groups of functions to their own file, and move classes to their own files.

We may need to consider back-compat issues, like plugins including wp-admin/includes/file.php in order to call a function that resides there in 4.9.6. Ideally this should be done in 5.0, to minimize the time where plugins will start to rely on functions being available in certain files.

[desrosj]

Moving to the new Privacy component.

[desrosj]

Marking this as 5.0, but to prevent complicating backports this should be done when 5.0 is confirmed as the next release.

[desrosj]

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

[desrosj]

Related: #44845.

[garrett-eclipse]

Some discussion on this happened in #44876

Adding Quotes for easier context here;
Comment 44876-6 - https://core.trac.wordpress.org/ticket/44876#comment:6

It also seems odd that the URL doesn't follow standard convention of using page as the param and then having wp-privacy-policy-guide as the value. The other two privacy related pages under tools follow this;
<URL>/wp-admin/tools.php?page=export_personal_data
<URL>/wp-admin/tools.php?page=remove_personal_data
*This change would require the tools.php code that loads this be updated as well.

Comment 44876-7 - https://core.trac.wordpress.org/ticket/44876#comment:7

And I guess another also valid option would be have the guide live in privacy.php instead of in tools.php as the guide is dedicated specifically to the settings in privacy.php and is currently only available from privacy.php

Comment 44876-8 - https://core.trac.wordpress.org/ticket/44876#comment:8

@garrett-eclipse interesting points you raise, but I think new admin files could be introduced in the future:
There's a restriction in place for minor point releases, like 4.9.6, that it must not create new files. So various existing files were hijacked in 4.9.6 to inject the Privacy related code :-)
New files can be introduced in 5.0, so a cleanup will be needed then.
As an example the export/erase request pages might get their own files, so the page url parameter would not be used in that case.
Regarding the Privacy guide, I wonder if it should also get it's own file?

Comment 44876-10 - https://core.trac.wordpress.org/ticket/44876#comment:10

Moving forward I feel the guide should just live in the privacy.php with the Privacy settings.

[garrett-eclipse]

Quoting @desrosj from #core-privacy Slack;
'some scenarios we need to consider. https://core.trac.wordpress.org/ticket/44845#comment:2 for example.'
Reference - ​https://wordpress.slack.com/archives/C9695RJBW/p1541007701145600

[desrosj]

Some ​discussion in Slack for this today.

The plan is as follows:

1. Create a map of where current code belongs.
2. The 5.0 code is merged into trunk create a patch (to limit patch revisions)
3. Commit this just before a 5.1 beta is set to land.

[desrosj]

This still needs an initial patch.

[garrett-eclipse]

While working on #46056 and #44721 I found it jarring that the two methods for handling privacy request fulfillment emails for export and erasure were quite different in location, naming convention and coding approach can we look to make them consistent when we re-org here.

[garrett-eclipse]

Related Note about #46061 is it introduced set_current_screen to overcome the fact those tables aren't in dedicated files but are subpages. When this re-org takes place the two instances of set_current_screen can be removed.

[garrett-eclipse]

As discussed in #core-privacy today moving this to Future Release as it doesn't have enough momentum to land in 5.2

[garrett-eclipse]

Now that #44047 is in trunk adding a note here to indicate when the export and erasure screens get dedicated pages their form actions will need to be updated accordingly.

[idea15]

Main core-privacy focus for 5.3.

[xkon]

Organize privacy code

[xkon]

43895.diff​ moves code around into the correct files and makes some necessary adjustments for the new pieces to work.

These include:

• Introduces Export / Erasure pages (erase-personal-data.php, export-personal-data.php )
• Adds them into the Menu ( please tell me if re-adjusting the priority in menu.php isn't something that we generally do so I can fix that :) )
• Removes the previous code from user.php
• Introduces privacy.php ( general privacy functions )
• Introduces class-wp-privacy-requests-table.php
• Introduces class-wp-privacy-data-removal-requests-table.php
• Introduces class-wp-privacy-data-export-requests-table.php
• Requires privacy.php into admin.php
• Removes the previous code from xfn.js and introduces privacy.js
• Adds privacy.js to Gruntfile.js
• Adds privacy.js to script-loader.php
• Adds localization for privacyToolsL10n to privacy.js instead of xfn.js
• Enqueues privacy.js instead of xfn.js into Export & Erase data pages.

I hope that I didn't miss anything, do tell me if I have to update any parts and I'll re-upload a fresh patch asap!

Props for this should go to @birgire and @garrett-eclipse as well please if committed :).

@azaozz / @desrosj , can you take a look on this when you get some time? Plenty of things to follow for Exports and other ideas for 5.3 so it'll be great to have the files finally done :D .

[desrosj]

Thanks for taking a first pass at this, everyone! I have only briefly skimmed the patch and did not test yet, but here are some initial thoughts:

• Was there a reason that the privacy JavaScript was in xfn.js? If so, xfn.js will need to be added as a dependency.
• class-wp-privacy-data-removal-requests-table.php and class-wp-privacy-data-export-requests-table.php need to be changed to class-wp-privacy-data-export-requests-list-table.php and class-wp-privacy-data-(removal erasure?)-requests-list-table.php respectively. This is for the reasons detailed here.
• The old classes will need to remain in core somewhere for backwards compatibility (erhaps in wp-admin/includes/deprecated.php). The old classes should throw a _doing_it_wrong() and load the new classes. something like this, maybe: Old_Class extends New_Class { function __construct() { doing_it_wrong(); parent::__construct(); } } (props @johnbillion for this example to try out). The loading order needs to be explored here to ensure this is a suitable option.
• There will most likely need to be function_exists() checks for any function reorganized when called. If the function does not exist, include_once should load privacy.php.
• Some code needs to be added to guarantee that the old privacy tool locations (tools.php?page=export_personal_data and tools.php?page=remove_personal_data) are redirected to the new locations. $_GET params and form submissions need to be tested to ensure everything is included successfully.
• Are there any front-end privacy related functions that warrant a wp-includes/privacy.php?

Also, it will probably be best to split this up into some more manageable chunks. What those chunks are though are not obvious to me without fully diving in as a lot of elements are very tightly connected.

[desrosj]

Also, adding the function_exists() checks is just a suggestion. There may be a different way code reorganization like this has been approached in the past. A more senior contributor may have more insight on that. Otherwise, some investigation could confirm/deny that as the best approach.

[azaozz]

This is a lot of moving.. :)

In 43895.1.diff:

• Rename privacy to privacy-tools.
• Move WP_Privacy_Policy_Content from wp-admin/includes/misc.php to wp-admin/includes/class-wp-privacy-policy-content.php.
• Rename the settings screen from wp-admin/privacy.php to wp-admin/options-privacy.php. wp-admin/privacy.php will need to be restored to its previous content in another commit.
• Add /wp-admin/privacy-policy-guide.php to output the guide. Previously this was done with a query arg from tools.php. Also add a redirect for it.

Would be great to avoid loading of class-wp-privacy-policy-content.php on every run. It has a big chunk of translated text that is very rarely used/needed. Currently there are actions that require it, perhaps can look into moving the callbacks for the actions to wp-admin/includes/privacy-tools.php?

[azaozz]

Replying to desrosj:

Was there a reason that the privacy JavaScript was in xfn.js?

Yes, as with the rest of the code, we couldn't add new files in a dot release. xfn.js was chosen as a "host" to temporarily hold the privacy tools js too.

class-wp-privacy-data-removal-requests-table.php and class-wp-privacy-data-export-requests-table.php need to be changed to class-wp-privacy-data-export-requests-list-table.php and class-wp-privacy-data-(removal erasure?)-requests-list-table.php respectively. This is for the reasons detailed here.

Don't see a problem with renaming them. Yes, the old classes can be added to deprecated.php

There will most likely need to be function_exists() checks for any function reorganized when called. If the function does not exist, include_once should load privacy.php.

Not seeing any functions that are not loaded. Can re-check the loading order just in case.

Some code needs to be added to guarantee that the old privacy tool locations (tools.php?page=export_personal_data and tools.php?page=remove_personal_data) are redirected to the new locations.

Right, will need to add that, same as for the privacy-policy-guide.php.

Are there any front-end privacy related functions that warrant a wp-includes/privacy.php?

Don't think that exists. May be missing something... Or do you mean we way need it? :)

Also, it will probably be best to split this up into some more manageable chunks.

May be possible but imho will be harder, need to make sure all separate "steps" of moving and renaming work by themselves.

[azaozz]

In 43895.2.diff:

• Renamed the classes to match the list table pattern (as above).
• Added the old classes to deprecated.php with some back-compat fixes.
• Added a helper function to WP_Privacy_Requests_Table to fix/normalize the admin URLs used in the list tables.
• Added the list table classes to _get_list_table() as per #44845.

Seems to be working well. Please test and lets add this :)

[birgire]

This looks good.

Maybe it's better to have as few as possible to modify and upload such a large patch, to minimize possible related problems :-)

So I don't upload a modified patch for now :-)

Few things I noticed when testing 43895.2.diff​:

We need to remove:

<input type="hidden" name="page" value="export_personal_data" />

from

src/wp-admin/export-personal-data.php

and

<input type="hidden" name="page" value="remove_personal_data" />

from

src/wp-admin/erase-personal-data.php

to have the request search working again.

Then we need to add

// Add screen options.
_wp_privacy_requests_screen_options();

above

// Handle list table actions.
_wp_personal_data_handle_actions();

in both src/wp-admin/export-personal-data.php and src/wp-admin/erase-personal-data.php.

But then we also need to change the following line in the private _wp_privacy_requests_screen_options() function in src/wp-admin/includes/privacy-tools.php:

'option' => str_replace( 'tools_page_', '', get_current_screen()->id ) . '_requests_per_page',

to preserve the existing options names: export_personal_data_requests_per_page and remove_personal_data_requests_per_page, if I recall correctly, as the screen ID has changed.

Or deprecate _wp_privacy_requests_screen_options() and add directly:

$args = array(
	'option'  => 'export_personal_data_requests_per_page',
);

add_screen_option( 'per_page', $args );

to

src/wp-admin/export-personal-data.php

and

$args = array(
	'option'  => 'remove_personal_data_requests_per_page',
);

add_screen_option( 'per_page', $args );

to

src/wp-admin/erase-personal-data.php

to preserve the existing pagination screen options.

[azaozz]

Replying to birgire:

Thanks for testing!

Maybe it's better to have as few as possible to modify..

Yeah, if we ever need to do something like this again (hope not!!), would try it in smaller chunks, even if it breaks things between the different parts.

Thinking to add the fixes/changes you found and commit so it's easier to test. Then we can add any fixes and tweaks.

[azaozz]

In 45448:

Privacy tools: Organize privacy functions into logical files and classes.

Props xkon, birgire, desrosj, garrett-eclipse, azaozz.
See #43895.

[azaozz]

In 45451:

Privacy tools: restore privacy.php to its "proper" use to output the Privacy tab on the About screen, see [42814]. Then add the Privacy tab updates from freedoms.php.

See #43895.

[azaozz]

In 45452:

Fix typo in [45451] and move the redirect to the proper location.

See #43895.

[SergeyBiryukov]

In 45453:

Privacy: Remove reinstated wp-admin/options-privacy.php from $_old_files.

Fix WPCS violations in [45448].

See #43895.

[SergeyBiryukov]

In 45454:

Docs: Add a comment about the reinstated wp-admin/options-privacy.php.

See #43895.

[garrett-eclipse]

Question: Should we take this opportunity to rename somethings properly. Speaking specifically of removal vs. erasure.
For example - WP_Privacy_Data_Removal_Requests_List_Table
Should we be updating them to the more appropriate erasure verbiage?
WP_Privacy_Data_Erasure_Requests_List_Table

For context, Erasure is the more appropriate term as a users content is erased (anonymized or removed) and not always removed as a removal also entails a user deletion which the tools don't currently support.

Thoughts? If it makes sense and doesn't have high back-compat overhead I'll create a patch.

[garrett-eclipse]

Another Question: Should the CSS for privacy tools and guide be isolated to their own .css files and be loaded only as needed?

Currently there's a few blocks of privacy css.

In edit.css;
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/css/edit.css#L658-L743
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/css/edit.css#L1649-L1662

In forms.css;
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/css/forms.css#L1099-L1233
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/css/forms.css#L1526-L1555

[birgire]

Replying to garrett-eclipse:

Question: Should we take this opportunity to rename somethings properly. Speaking specifically of removal vs. erasure.
For example - WP_Privacy_Data_Removal_Requests_List_Table
Should we be updating them to the more appropriate erasure verbiage?
WP_Privacy_Data_Erasure_Requests_List_Table

For context, Erasure is the more appropriate term as a users content is erased (anonymized or removed) and not always removed as a removal also entails a user deletion which the tools don't currently support.

Thoughts? If it makes sense and doesn't have high back-compat overhead I'll create a patch.

Good question.

Since we are already deprecating the WP_Privacy_Data_Removal_Requests_Table class, now would be a good time to do this.

I'm in favor of it, if there are no obvious complications, since the erase vs remove case can sometimes be confusing:

One question that comes to mind if we would need to deprecate the current WP_Privacy_Data_Removal_Requests_List_Table class, in trunk, if it hasn't been a part of any shipped version yet?

PS: There are some CSS classes with the remove- prefix in WP_Privacy_Data_Removal_Requests_List_Table. That might be considered in another ticket if needed. Then there's the request type: remove_personal_data, that I think will never change.

[azaozz]

There are some CSS classes with the remove- prefix in WP_Privacy_Data_Removal_Requests_List_Table. That might be considered in another ticket if needed. Then there's the request type: remove_personal_data, that I think will never change.

I actually had the first iteration of the patch with renamed class. But yes, many places in the code refer to remove and removal, from settings saved in the DB to HTML class names. Cannot change all as it would introduce edge cases/regressions. We can rename the class but... It will make it (perhaps) even more inconsistent?

[azaozz]

Should the CSS for privacy tools and guide be isolated to their own .css files

Good question. The only stand-alone file that may need that is (the new) privacy-policy-guide.php. The other screens are part of "settings" which is (mostly) forms.css and list tables in list-tables.css.

It will perhaps make sense to move the CSS for the privacy policy guide out of edit.css to a separate file. On the other hand it's just a few lines, maybe better to leave it as-is or to add it to common.css?

[azaozz]

Looking through this again, found some left-over "dead" code and couple of inline comments that need updating/fixing.

Also now that the "big move" seems to be working well, next step is to move the helper functions mostly from wp-admin/includes/file.php to wp-admin/includes/privacy-tools.php. Patch coming up.

Also, need to move WP_User_Request to a its own file, and perhaps move the user request handling functions from wp-includes/user.php into a separate file.

[azaozz]

In 45500:

Privacy tools: remove some left-over code and fix inline comment.

See #43895.

[azaozz]

In 43895.3.diff:

• Move the privacy related functions from wp-admin/includes/file.php to wp-admin/includes/privacy-tools.php.
• Move the WP_User_Request class to a separate file.

Wondering if we should also move the _wp_privacy-* and user request related functions from wp-includes/user.php: ​https://github.com/WordPress/wordpress-develop/blob/0af549fdf50ac779fb68cc0bd688ac283d5195ec/src/wp-includes/user.php#L2840-L3650. They are user related, but for both registered and non-registered (website) users. Perhaps user.php is a good place anyway.

[azaozz]

In 45519:

Privacy tools:

• Move the (remaining) privacy tools related functions from wp-admin/includes/file.php to wp-admin/includes/privacy-tools.php.
• Move the WP_User_Request class to a separate file.

See #43895.

[azaozz]

With [45519] thinking this is almost done. The only thing remaining is to perhaps move the privacy policy guide CSS.

[desrosj]

I am going to close this one out. I couldn't find any CSS on the privacy policy guide page, so I think that may have been moved separately. Let's address remaining issues in new tickets. Thank you for your work on this, everyone!

[SergeyBiryukov]

Replying to desrosj:

I am going to close this one out. I couldn't find any CSS on the privacy policy guide page, so I think that may have been moved separately. Let's address remaining issues in new tickets. Thank you for your work on this, everyone!

That was about moving the styles from edit.css and forms.css to a separate file, see comment:45 and comment:48. Those styles are still there, I don't have a strong opinion on moving them though.

[garrett-eclipse]

Replying to azaozz:

Wondering if we should also move the _wp_privacy-* and user request related functions from wp-includes/user.php: ​https://github.com/WordPress/wordpress-develop/blob/0af549fdf50ac779fb68cc0bd688ac283d5195ec/src/wp-includes/user.php#L2840-L3650. They are user related, but for both registered and non-registered (website) users. Perhaps user.php is a good place anyway.

Hi @azaozz sorry to resurface this ticket. I was working on #46536 and found the _wp_privacy_action_request_types function remains in the user.php but is only ever referenced from the privacy-tools.php and was wondering if we should move that function there to keep it contained within the file it's reference from.

Also I do like the idea of isolating the user privacy functions, maybe into a user-privacy.php. But that being said user.php is also a valid location as they are user related.