#43895 closed enhancement (fixed)
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 |
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)
Change History (61)
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
7 years ago
#3
@
7 years 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
@
6 years ago
- Keywords gdpr removed
Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.
This ticket was mentioned in Slack in #core-privacy by birgire. View the logs.
6 years ago
#8
@
6 years 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.
#9
@
6 years 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
#10
@
6 years ago
Some discussion in Slack for this today.
The plan is as follows:
- Create a map of where current code belongs.
- The 5.0 code is merged into
trunk
create a patch (to limit patch revisions) - 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.
6 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
6 years ago
#19
@
6 years 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 years ago
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
6 years ago
#22
@
6 years 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
@
6 years 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.
6 years ago
This ticket was mentioned in Slack in #core-privacy by birgire. View the logs.
6 years ago
#27
@
6 years 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.
6 years ago
#29
follow-up:
↓ 32
@
6 years 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
andclass-wp-privacy-data-export-requests-table.php
need to be changed toclass-wp-privacy-data-export-requests-list-table.php
andclass-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 loadprivacy.php
. - Some code needs to be added to guarantee that the old privacy tool locations (
tools.php?page=export_personal_data
andtools.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
@
6 years 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.
#31
@
6 years ago
This is a lot of moving.. :)
In 43895.1.diff:
- Rename
privacy
toprivacy-tools
. - Move
WP_Privacy_Policy_Content
fromwp-admin/includes/misc.php
towp-admin/includes/class-wp-privacy-policy-content.php
. - Rename the settings screen from
wp-admin/privacy.php
towp-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
@
6 years 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
andclass-wp-privacy-data-export-requests-table.php
need to be changed toclass-wp-privacy-data-export-requests-list-table.php
andclass-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 loadprivacy.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
andtools.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.
#33
@
6 years 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:
↓ 35
@
6 years 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
@
6 years 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.
This ticket was mentioned in Slack in #core-privacy by birgire. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-privacy by azaozz. View the logs.
6 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
#44
follow-up:
↓ 46
@
6 years 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.
#45
@
6 years ago
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
#46
in reply to:
↑ 44
@
6 years 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?
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.
#47
@
6 years 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?
#48
@
6 years 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
@
6 years 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.
#51
follow-up:
↓ 57
@
6 years ago
In 43895.3.diff:
- Move the privacy related functions from
wp-admin/includes/file.php
towp-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.
#53
@
5 years 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.
5 years ago
#55
follow-up:
↓ 56
@
5 years ago
- Keywords needs-testing removed
- Resolution set to fixed
- Status changed from assigned to closed
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!
#56
in reply to:
↑ 55
@
5 years ago
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.
#57
in reply to:
↑ 51
@
5 years ago
Replying to azaozz:
Wondering if we should also move the
_wp_privacy-*
and user request related functions fromwp-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. Perhapsuser.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.
Moving to the new Privacy component.