Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#44876 closed enhancement (fixed)

Same url for the Privacy Policy Guide

Reported by: birgire's profile birgire Owned by: garrett-eclipse's profile 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 6 years ago.
Try this patch to remove query string for privacy policy page.
44876.diff (1.2 KB) - added by mukesh27 5 years ago.
Updated Patch.
44876.2.diff (702 bytes) - added by garrett-eclipse 5 years ago.

Download all attachments as: .zip

Change History (29)

#1 @birgire
6 years ago

  • Keywords good-first-bug added

I mark this as a good first bug.

#2 @birgire
6 years ago

  • Description modified (diff)

#3 @mukesh27
6 years ago

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

@mukesh27
6 years ago

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

#4 @mukesh27
6 years ago

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

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

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


6 years ago

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


5 years ago

#12 @garrett-eclipse
5 years 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 years ago

#14 @garrett-eclipse
5 years ago

  • Keywords good-first-bug removed

#15 @desrosj
5 years ago

  • Milestone changed from Awaiting Review to Future Release

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

  • Milestone changed from Future Release to 5.2

#18 @garrett-eclipse
5 years ago

  • Keywords needs-refresh added

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

@mukesh27
5 years ago

Updated Patch.

#19 @mukesh27
5 years ago

  • Keywords needs-refresh removed

#20 @garrett-eclipse
5 years 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
5 years 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
5 years 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
5 years 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 years ago

#25 @garrett-eclipse
5 years ago

  • Status changed from assigned to accepted

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