WordPress.org

Make WordPress Core

Opened 8 months ago

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

Download all attachments as: .zip

Change History (29)

#1 @birgire
8 months ago

  • Keywords good-first-bug added

I mark this as a good first bug.

#2 @birgire
8 months ago

  • Description modified (diff)

#3 @mukesh27
8 months ago

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

@mukesh27
8 months ago

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

#4 @mukesh27
8 months ago

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

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

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


8 months ago

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


6 months ago

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


5 months ago

#14 @garrett-eclipse
3 months ago

  • Keywords good-first-bug removed

#15 @desrosj
2 months ago

  • Milestone changed from Awaiting Review to Future Release

#16 @garrett-eclipse
8 weeks 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
8 weeks ago

  • Milestone changed from Future Release to 5.2

#18 @garrett-eclipse
8 weeks ago

  • Keywords needs-refresh added

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

@mukesh27
8 weeks ago

Updated Patch.

#19 @mukesh27
8 weeks ago

  • Keywords needs-refresh removed

#20 @garrett-eclipse
8 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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.


5 weeks ago

#25 @garrett-eclipse
5 weeks ago

  • Status changed from assigned to accepted

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