Opened 6 years ago
Closed 6 years 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 )
There are currently two versions of the Privacy Policy Guide url in use:
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)
Change History (29)
#3
@
6 years ago
- Severity changed from normal to minor
- Type changed from defect (bug) to enhancement
#5
@
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
@
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
#7
@
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
@
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
@
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.
6 years ago
#12
@
6 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.
6 years ago
#16
@
6 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.
#18
@
6 years ago
- Keywords needs-refresh added
#44761 has been committed thanks @SergeyBiryukov
@mukesh27 did you want to refresh your patch against trunk?
#20
@
6 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
@
6 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
@
6 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
@
6 years ago
@desrosj thanks for the review. @garrett-eclipse patch 44876.2.diff is working fine for me.
I mark this as a good first bug.