Privacy: Pagination screen options for the requests list tables

[birgire]

It's handy to have the screen options for pagination.

We could add it with e.g.:

$args = array(
    'label'   => __( 'Requests per page' ),
    'default' => 20,
    'option'  => $screen->id . '_requests_per_page'   
);
add_screen_option( 'per_page', $args );

or with a shorter options like export_requests_per_page and erase_requests_per_page.

We then need to adjust the switch in set_screen_options() to handle these two new options.

I assume the admin pages:

wp-admin/tools.php?page=export_personal_data
wp-admin/tools.php?page=remove_personal_data

will get their own files in 5.0?

Else we would need to hook into the corresponding "load-{$page_hook}" action.

Then we can fetch the per page option with:

$posts_per_page = $this->get_items_per_page( $this->screen->id . '_requests_per_page' );

in the WP_Privacy_Requests_Table::prepare_items() method.

[desrosj]

Moving gdpr tickets that are not bugs to Future Release until the next steps can be properly evaluated.

[desrosj]

Moving to the new Privacy component.

[birgire]

44025.diff​ adds the screen pagination options:

• export_personal_data_requests_per_page
• remove_personal_data_requests_per_page

with the hooks:

• load-tools_page_export_personal_data
• load-tools_page_remove_personal_data

since these pages (still) don't have dedicated admin files.

[ocean90]

4.9.7 has been released, moving to next milestone.

[desrosj]

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

[pbiron]

44025.1.diff fixes a couple of issues with 44025.diff:

1. typo in action in /wp-admin/includes/admin-filters.php
   • load-tools_page_removal_personal_data => load-tools_page_remove_personal_data
2. incorrect method call to get $posts_per_page in WP_Privacy_Requests_Table::prepare_items()
   • $this->get_pagination_arg(...); => $this->get_items_per_page(...)

I also couldn't find anywhere else in core that sets a label for the per_page screen option (a few plugins I use set it, but nowhere in core), so I removed that.

I'd appreciate someone testing this patch.

[desrosj]

44025.1.diff​ tests well for me! Few small points before I think it can be considered ready:

• The @since tag needs to be changed to 4.9.8.
• get_current_screen() can return null if the $current_screen global is not set. Looking at other classes that extend WP_List_Table, they utilize the $screen property of WP_List_Table. This may be a better option as it takes a few extra steps to ensure that property has a value.

[pbiron]

Replying to desrosj:

44025.1.diff​ tests well for me! Few small points before I think it can be considered ready:

• The @since tag needs to be changed to 4.9.8.

oops!

• get_current_screen() can return null if the $current_screen global is not set. Looking at other classes that extend WP_List_Table, they utilize the $screen property of WP_List_Table. This may be a better option as it takes a few extra steps to ensure that property has a value.

Unfortunately, the WP_Privacy_Requests_Table isn't yet instantiated when _wp_privacy_requests_screen_options() is called so using its $screen property isn't an option.

As far as I can tell, get_current_screen() returning null is a non-issue since set_current_screen() is called (/wp-admin/admin.php#L203) just before do_action( "load-{$page_hook}" ); (/wp-admin/admin.php#L228), which is what causes _wp_privacy_requests_screen_options() to be called.

I'll refresh the patch with the correct @since 4.9.8 once someone else confirms my sense on get_current_screen() being safe to call.

[birgire]

Thanks @pbiron and @desrosj

doh, it looks like the patch 44025.diff​ I uploaded wasn't the patch I intended, sorry about that :) I remember testing the pagination successfully and I can see that in the ticket's description I mentioned using $this->get_items_per_page() but my uploaded patch included $this->get_pagination_arg(), most likely from some earlier testing that also included a filter typo :)

ps: I also remember wanting to use the 'paged' query parameter instead of calculating 'offset', in WP_Privacy_Requests_Table::prepare_items(), but that's unrelated :)

[birgire]

yes that's was my understanding too regarding the screen setup, just before the hook fires.

I agree with removing the labeling since it's not used in core, good catch.

I think we can also remove the 'default' => 20 and use the fallback, like other tables.

[pbiron]

Replying to birgire:

yes that's was my understanding too regarding the screen setup, just before the hook fires.

thanx for the confirmation.

I think we can also remove the 'default' => 20 and use the fallback, like other tables.

I thought about that as well, but then noticed that /wp-admin/edit.php#:292 and /wp-admin/edit-tags.php#L57 explicitly set 'default' => 20, so I left it in. I can take it out if you feel strongly, but I suggest leaving it in.

[birgire]

Replying to pbiron:

I think we can also remove the 'default' => 20 and use the fallback, like other tables.

I thought about that as well, but then noticed that /wp-admin/edit.php#:292 and /wp-admin/edit-tags.php#L57 explicitly set 'default' => 20, so I left it in. I can take it out if you feel strongly, but I suggest leaving it in.

---

Seems to be both ways in core, like with the comments table, that's not setting it manually.

https://core.trac.wordpress.org/browser/tags/4.9.7/src/wp-admin/edit-comments.php#L163

---

The fallback is 20 in WP_Screen::render_per_page_options():

https://core.trac.wordpress.org/browser/tags/4.9.7/src/wp-admin/includes/class-wp-screen.php#L1133

and same in protected function get_items_per_page( $option, $default = 20 ) {

https://core.trac.wordpress.org/browser/tags/4.9.7/src/wp-admin/includes/class-wp-list-table.php#L683

So we're currently setting the default value manually for render_per_page_options() but not manually for the second input parameter of get_items_per_page() method.

If we were changing the value to something different than 20, then we would need to set both.

But I don't have strong opinion regarding this in our case :-) so I'm open to your suggestion.

[pbiron]

update @since -> 4.9.8 and remove 'default' => 20

[pbiron]

Replying to birgire:

If we were changing the value to something different than 20, then we would need to set both.

OK, I'm convinced.

44025.2.diff removes 'default' => 20 and updates @since to 4.9.8.

I think this is ready.

[audrasjb]

I tested this patch and it seems to work as expected.

[SergeyBiryukov]

In 43486:

Privacy: Enable pagination screen options for privacy requests list tables.

Props birgire, pbiron.
Fixes #44025.

[SergeyBiryukov]

In 43487:

Privacy: Enable pagination screen options for privacy requests list tables.

Props birgire, pbiron.
Merges [43486] to the 4.9 branch.
Fixes #44025.