WordPress.org

Make WordPress Core

#44876 closed enhancement (fixed)

Same url for the Privacy Policy Guide

Reported by: birgire Owned by: garrett-eclipse
Milestone: 5.2 Priority: low
Severity: minor Version: 4.9.6
Component: Privacy Keywords: has-patch needs-testing commit
Focuses: administration Cc:

Description (last modified by birgire)

There are currently two versions of the Privacy Policy Guide url in use:

We have both src + src:

admin_url( 'tools.php?wp-privacy-policy-guide=1' ) 

and src:

admin_url( 'tools.php?wp-privacy-policy-guide' ) 

I think we should use the same url for consistency.

This is the check in tools.php (src):

$is_privacy_guide = ( isset( $_GET['wp-privacy-policy-guide'] ) && current_user_can( 'manage_privacy_options' ) );

Attachments (3)

44876.patch (1.1 KB) - added by mukesh27 23 months ago.
Try this patch to remove query string for privacy policy page.
44876.diff (1.2 KB) - added by mukesh27 17 months ago.
Updated Patch.
44876.2.diff (702 bytes) - added by garrett-eclipse 16 months ago.

Download all attachments as: .zip

Change History (29)

#1 @birgire
23 months ago

  • Keywords good-first-bug added

I mark this as a good first bug.

#2 @birgire
23 months ago

  • Description modified (diff)

#3 @mukesh27
23 months ago

  • Severity changed from normal to minor
  • Type changed from defect (bug) to enhancement

@mukesh27
23 months ago

Try this patch to remove query string for privacy policy page.

#4 @mukesh27
23 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

#5 @birgire
23 months ago

  • Owner set to mukesh27
  • Status changed from new to assigned

Thanks for the patch @mukesh27, the 44876.patch looks good.

I don't have a preferred version here, but the isset( $_GET['wp-privacy-policy-guide'] ) check should catch both tools.php?wp-privacy-policy-guide and tools.php?wp-privacy-policy-guide=1.

I mark it as claimed by setting you as the owner.

#6 @garrett-eclipse
23 months ago

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.

@birgire let me know if I should isolate the above concern in it's own ticket.

Thanks

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

#7 @garrett-eclipse
23 months ago

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

#8 @birgire
23 months ago

@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?

I haven't checked if there are existing tickets for this cleanup.

I think it would be good to add this as an agenda point to a future weekly chat, how best to prepare for this and locate what needs to be moved and what files need to be created.

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


23 months ago

#10 @garrett-eclipse
23 months ago

Thanks @birgire I requested an addition to the future agenda to discuss.

Makes sense not to introduce new files, was just feeling the guide could follow the same URL format as the other privacy pages as all the files needed for it exist already. With that format we wouldn't have a valueless query param.

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

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


21 months ago

#12 @garrett-eclipse
21 months ago

@birgire note the cleanup of files is being done by @allendav in #43895
Your input is appreciated. He's started working on it in a Doc here - https://docs.google.com/document/d/1naG6Bs2RK7j1PkKXmCdflY3ei2ZgZtV6zHCWeRz01s0/edit?usp=sharing

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


20 months ago

#14 @garrett-eclipse
18 months ago

  • Keywords good-first-bug removed

#15 @desrosj
17 months ago

  • Milestone changed from Awaiting Review to Future Release

#16 @garrett-eclipse
17 months ago

#44761 is currently blocking this ticket as it affects the same line of code. I've marked it for commit in 5.2 so once it's in trunk will apply a refresh here.

#17 @SergeyBiryukov
17 months ago

  • Milestone changed from Future Release to 5.2

#18 @garrett-eclipse
17 months ago

  • Keywords needs-refresh added

#44761 has been committed thanks @SergeyBiryukov
@mukesh27 did you want to refresh your patch against trunk?

@mukesh27
17 months ago

Updated Patch.

#19 @mukesh27
17 months ago

  • Keywords needs-refresh removed

#20 @garrett-eclipse
17 months ago

  • Keywords commit added; needs-testing removed

Thanks @mukesh27 appreciate the refresh.

Looks good; applies cleanly; clears tests; renders and links properly.
*Note for testers, the notice currently needs Classic Editor due to #46098

Marking for commit.

#21 @desrosj
16 months ago

  • Keywords needs-refresh added; commit removed

Looking at this today, I think I would prefer to keep the 1 value in the URL. Core is only using isset(), however, a plugin or theme could be using if ( ! empty( $_GET['wp-privacy-policy-guide'] ) ), or checking the value is equal to 1.

#22 @garrett-eclipse
16 months ago

  • Keywords needs-testing commit added; needs-refresh removed
  • Owner changed from mukesh27 to garrett-eclipse

Thanks for the review @desrosj and the catch. Good point on the use of empty, I myself prefer ! empty over isset in most cases. I've replaced the diff and moved it from removing =1 from the two instances to adding =1 to the single instance. Please re-review and we can commit. Cheers

#23 @mukesh27
16 months ago

@desrosj thanks for the review. @garrett-eclipse patch 44876.2.diff is working fine for me.

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


16 months ago

#25 @garrett-eclipse
16 months ago

  • Status changed from assigned to accepted

#26 @SergeyBiryukov
16 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 44971:

Privacy: Use a consistent URL for Privacy Policy guide.

Props garrett-eclipse, mukesh27, birgire, desrosj.
Fixes #44876.

Note: See TracTickets for help on using tickets.