WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 2 weeks ago

#43895 assigned enhancement

Organize privacy functions into logical files and classes

Reported by: iandunn Owned by: desrosj
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: needs-patch early
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.

Change History (23)

#1 @desrosj
11 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.


11 months ago

#3 @desrosj
11 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
10 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
7 months ago

Related: #44845.

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


7 months ago

#7 @SergeyBiryukov
6 months ago

  • Milestone changed from 5.0 to 5.1

#8 @garrett-eclipse
6 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 8 weeks ago by garrett-eclipse (previous) (diff)

#9 @garrett-eclipse
6 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 6 months ago by garrett-eclipse (previous) (diff)

#10 @desrosj
6 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.


4 months ago

#12 @desrosj
4 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.


3 months ago

#14 @garrett-eclipse
3 months ago

  • Version set to 4.9.6

#15 @desrosj
3 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.


2 months ago

#17 @garrett-eclipse
2 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.


8 weeks ago

#19 @garrett-eclipse
7 weeks 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.


7 weeks ago

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


6 weeks ago

#22 @garrett-eclipse
6 weeks 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
2 weeks 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.

Note: See TracTickets for help on using tickets.