WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 4 months ago

#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 11 months ago.
Try this patch to remove query string for privacy policy page.
44876.diff (1.2 KB) - added by mukesh27 5 months ago.
Updated Patch.
44876.2.diff (702 bytes) - added by garrett-eclipse 4 months ago.

Download all attachments as: .zip

Change History (29)

#1 @birgire
11 months ago

  • Keywords good-first-bug added

I mark this as a good first bug.

#2 @birgire
11 months ago

  • Description modified (diff)

#3 @mukesh27
11 months ago

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

@mukesh27
11 months ago

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

#4 @mukesh27
11 months ago

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

#5 @birgire
11 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
11 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 11 months ago by garrett-eclipse (previous) (diff)

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


11 months ago

#10 @garrett-eclipse
11 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.


9 months ago

#12 @garrett-eclipse
9 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.


8 months ago

#14 @garrett-eclipse
6 months ago

  • Keywords good-first-bug removed

#15 @desrosj
5 months ago

  • Milestone changed from Awaiting Review to Future Release

#16 @garrett-eclipse
5 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
5 months ago

  • Milestone changed from Future Release to 5.2

#18 @garrett-eclipse
5 months ago

  • Keywords needs-refresh added

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

@mukesh27
5 months ago

Updated Patch.

#19 @mukesh27
5 months ago

  • Keywords needs-refresh removed

#20 @garrett-eclipse
5 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
4 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
4 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
4 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.


4 months ago

#25 @garrett-eclipse
4 months ago

  • Status changed from assigned to accepted

#26 @SergeyBiryukov
4 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.