WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 4 weeks ago

#43895 assigned enhancement

Organize privacy functions into logical files and classes

Reported by: iandunn Owned by: desrosj
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: early has-patch needs-testing
Focuses: Cc:

Description

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.

Attachments (4)

43895.diff (92.5 KB) - added by xkon 3 months ago.
Organize privacy code
43895.1.diff (183.5 KB) - added by azaozz 3 months ago.
43895.2.diff (188.8 KB) - added by azaozz 3 months ago.
43895.3.diff (37.9 KB) - added by azaozz 2 months ago.

Download all attachments as: .zip

Change History (58)

#1 @desrosj
15 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


15 months ago

#3 @desrosj
15 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.0

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

#4 @desrosj
14 months ago

  • Keywords gdpr removed

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

#5 @desrosj
11 months ago

Related: #44845.

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


11 months ago

#7 @SergeyBiryukov
10 months ago

  • Milestone changed from 5.0 to 5.1

#8 @garrett-eclipse
10 months ago

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.

Last edited 6 months ago by garrett-eclipse (previous) (diff)

#9 @garrett-eclipse
10 months ago

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

Last edited 10 months ago by garrett-eclipse (previous) (diff)

#10 @desrosj
10 months ago

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.

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


8 months ago

#12 @desrosj
7 months ago

  • Milestone changed from 5.1 to 5.2

This still needs an initial patch.

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


7 months ago

#14 @garrett-eclipse
7 months ago

  • Version set to 4.9.6

#15 @desrosj
7 months ago

  • Keywords early added
  • Owner set to desrosj
  • Status changed from new to assigned

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


6 months ago

#17 @garrett-eclipse
6 months ago

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.

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


6 months ago

#19 @garrett-eclipse
6 months ago

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.

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


6 months ago

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


5 months ago

#22 @garrett-eclipse
5 months ago

  • Milestone changed from 5.2 to Future Release

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

#23 @garrett-eclipse
4 months ago

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.

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


3 months ago

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


3 months ago

#26 @idea15
3 months ago

  • Milestone changed from Future Release to 5.3

Main core-privacy focus for 5.3.

@xkon
3 months ago

Organize privacy code

#27 @xkon
3 months ago

  • Keywords has-patch added; needs-patch removed

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 .

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


3 months ago

#29 follow-up: @desrosj
3 months ago

  • Keywords needs-refresh added

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.

#30 @desrosj
3 months ago

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
3 months ago

#31 @azaozz
3 months ago

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?

#32 in reply to: ↑ 29 @azaozz
3 months ago

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.

Last edited 3 months ago by azaozz (previous) (diff)

@azaozz
3 months ago

#33 @azaozz
3 months ago

  • Keywords needs-testing added; needs-refresh removed

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 :)

#34 follow-up: @birgire
3 months ago

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.

#35 in reply to: ↑ 34 @azaozz
3 months ago

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.

#36 @azaozz
3 months ago

In 45448:

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

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

#37 @azaozz
3 months ago

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.

#38 @azaozz
3 months ago

In 45452:

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

See #43895.

#39 @SergeyBiryukov
3 months ago

In 45453:

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

Fix WPCS violations in [45448].

See #43895.

#40 @SergeyBiryukov
3 months ago

In 45454:

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

See #43895.

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


3 months ago

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


3 months ago

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


3 months ago

#44 follow-up: @garrett-eclipse
2 months ago

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.

#46 in reply to: ↑ 44 @birgire
2 months ago

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?

Version 0, edited 2 months ago by birgire (next)

#47 @azaozz
2 months ago

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?

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

#48 @azaozz
2 months ago

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?

#49 @azaozz
2 months ago

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.

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

#50 @azaozz
2 months ago

In 45500:

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

See #43895.

@azaozz
2 months ago

#51 @azaozz
2 months ago

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.

#52 @azaozz
2 months ago

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.

#53 @azaozz
2 months ago

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

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


4 weeks ago

Note: See TracTickets for help on using tickets.