Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#45954 closed feature request (fixed)

Add a extra_tablenav() method to WP_MS_Sites_List_Table

Reported by: pbiron's profile pbiron Owned by: desrosj's profile desrosj
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.1
Component: Networks and Sites Keywords: has-screenshots has-patch has-dev-note
Focuses: administration, multisite Cc:

Description

With blogmeta landing in 5.1-beta1 (#37923 and #40229) it would be really sweet if WP_MS_Sites_List_Table had an extra_tablenav() method which fired restrict_manage_sites and manage_sites_extra_tablenav actions (analogous to WP_Posts_List_Table's restrict_manage_posts and manage_posts_extra_tablenav).

This would allow plugins to add dropdowns (or other UI components) to limit the sites shown in the list table to those that have specific values in blogmeta (just like is now possible with other WP_List_Table sub-classes).

I'm currently developing a large multisite (2000-3000 sites). The main site will serve as a directory of places of worship (searchable on the front-end by denomination and various other characteristics; blogmeta is going to make that searching much easier than it otherwise would have been!). Each place of worship will be a site in the network.

Having WP_MS_Sites_List_Table::extra_tablenav() would make the site admin's job a lot easier, as it would allow them to perform some bulk action on just those sites with a specific denomination, etc. I'll add a screenshot of a mockup once I finish creating this ticket.

I raised the possibility of this feature a few weeks ago in #core-multisite at a time when I didn't think blogmeta was going to land. I know that technically now that 5.1-beta1 is out there are not supposed to be enhancements or feature requests added...but I'm hoping we can bend the rules a little and get this in beta2. If that is possible, I can have a patch ready in a few days.

Attachments (5)

restrict_manage_sites.png (45.7 KB) - added by pbiron 6 years ago.
mockup of a use of the proposed 'restrict_manage_sites' action
49594.diff (2.8 KB) - added by pbiron 6 years ago.
45954.2.diff (2.8 KB) - added by pbiron 5 years ago.
same as 45954.diff but with @since updated to 5.3.0.
45954.diff (2.2 KB) - added by desrosj 5 years ago.
45954.3.diff (1.2 KB) - added by pbiron 5 years ago.

Download all attachments as: .zip

Change History (36)

@pbiron
6 years ago

mockup of a use of the proposed 'restrict_manage_sites' action

#1 @pbiron
6 years ago

  • Keywords has-screenshots added

#2 @pbiron
6 years ago

  • Version set to trunk

#3 @pento
6 years ago

  • Milestone changed from Awaiting Review to Future Release

Thank you for the suggestion, @pbiron! It's too late for 5.1, but if you want to put a patch together, we can look at it for 5.2. 🙂

#4 @pbiron
6 years ago

Yeah, I figured it was too late, but didn't hurt to ask.

I've got the basic patch ready, but since it's not needed immediately, I'll work on it a little more before posting it...including adding unit tests.

This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.


6 years ago

@pbiron
6 years ago

#6 @pbiron
6 years ago

49594.diff adds the WP_MS_Sites_List_Table::extra_table() method and the new restrict_manage_sites and manage_sites_extra_tablenav filters. It also changes /wp-admin/network/sites.php to process submissions of the "Filter" button.

I've been using (some version of) this patch since the first 5.1-beta came out on a couple of in development multisites and haven't noticed any problems.

#7 @pbiron
6 years ago

  • Keywords has-patch added

#8 follow-up: @pbiron
6 years ago

I was planning on adding some unit tests, but discovered there are no tests for the equivalent filters provided by other list tables so I didn't bother with any here.

The only tests I can find related to extra_tablenav() are for WP_Posts_List_Table (e.g.,
Tests_Admin_includesListTable::test_filter_button_should_not_be_shown_if_there_are_no_posts(), etc). I could add a test similar to those that checks that the "Filter" button is present if nothing hooks into the new filters, but I don't think that is really necessary.

This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by pbiron. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


5 years ago

#15 @spacedmonkey
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 5.3

@pbiron It is time for this to go into core. Can you update the since, I will see if I can get this merged.

#16 @pbiron
5 years ago

sure...I'll try to get to that later today.

@pbiron
5 years ago

same as 45954.diff but with @since updated to 5.3.0.

#17 @pbiron
5 years ago

  • Keywords needs-refresh removed

45954.2.diff is the same as 45954.diff but with @since updated to 5.3.0.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


5 years ago

#19 in reply to: ↑ 8 @pbiron
5 years ago

Replying to pbiron:

I was planning on adding some unit tests, but discovered there are no tests for the equivalent filters provided by other list tables so I didn't bother with any here.

The only tests I can find related to extra_tablenav() are for WP_Posts_List_Table (e.g.,
Tests_Admin_includesListTable::test_filter_button_should_not_be_shown_if_there_are_no_posts(), etc). I could add a test similar to those that checks that the "Filter" button is present if nothing hooks into the new filters, but I don't think that is really necessary.

With 5.3 beta 1 due in 1 week (Sep 23), can we get this committed?

Should I add 1 or more unit tests similar to the ones for WP_Posts_List_Table?

#20 @desrosj
5 years ago

  • Owner set to desrosj
  • Status changed from new to reviewing

@desrosj
5 years ago

#21 @desrosj
5 years ago

45954.diff has a few changes:

  • Coding standards fixes.
  • Removes () for the exit call (there should be a separate ticket to remove all of these since it is a language construct and does not require them)

I also looked at how the other list tables handle these requests. Between edit.php, users.php, and edit-comments.php, there are some inconsistencies.

  • Some use $_GET, some use $_REQUEST` for the conditional.
  • Some use $_REQUEST some use $_SERVER for building the redirect URL.
  • Both use wp_redirect() instead of wp_safe_redirect().

I've updated the patch to be more consistent with these, even though there are some differences.

#22 @pbiron
5 years ago

The latest patch works great!

I think this is ready for commit!

Last edited 5 years ago by pbiron (previous) (diff)

#23 @desrosj
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 46211:

Networks and Sites: Add support for the extra_tablenav() method in WP_MS_Sites_List_Table.

This method allows additional filters or other UI components to be added to the top and bottom of the WP_List_Table between the bulk actions dropdown and search input field.

Fixes #45954.
Props pbiron, desrosj.

#24 @desrosj
5 years ago

In 46212:

Docs: Add missing $which parameter descriptions.

Follow up of [46211].

See #45954.

#25 @pbiron
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Upon further testing, what was committed in 46211 doesn't correctly process the query defined by WP_MS_Sites_List_Table::extra_tablenav().

  • in network/sites.php, the conditional at L234 should have used the $_POST super global instead of $_GET. This caused that branch of code to never execute. Without that wp_redirect() happening, the "filter" operation a user does results in the sites being filtered correctly. However, paginating the table after a filter operation results in the infamous The link you followed has expired. error screen.
  • with that conditional corrected, using $_SERVER['REQUEST_URI'] as the basis for constructing the URL to redirect also results in the The link you followed has expired. error screen when doing a "filter" operation, because when $_GET['allblogs'] is set (which it is when using $_SERVER['REQUEST_URI'] to construct the redirect URL) a nonce is checked which does not exist. The redirect needs to include all of $_POST (minus _wp_http_referer and _wpnonce), as was done in the original patch.

I wrote the original patch so long ago that I don't remember why I was checking the conditional in the 1st bullet point above, but it turns out that conditional doesn't even need to be there and the redirect can simply be in an else.

New patching coming shortly that corrects the above.

@pbiron
5 years ago

#26 @pbiron
5 years ago

45954.3.diff implements the fix described in #comment:25.

@desrosj when you get a chance, can you look at the new patch?

#27 @desrosj
5 years ago

  • Keywords commit added

Thanks for testing and catching this, @pbiron. The patch looks good.

#28 @desrosj
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 46384:

Networks and Sites: Fix issues processing additional fields displayed for the Sites list table.

This prevents a The link you followed has expired. error when using a filter and now uses $_POST instead of $_GET to capture all form values.

Props pbiron.
Fixes #45954.

This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.


5 years ago

#30 @justinahinon
5 years ago

  • Keywords needs-dev-note added
Note: See TracTickets for help on using tickets.