[wp-trac] [WordPress Trac] #37392: Multisite "Sites" screen: Add links to filter websites by status

[WordPress Trac]

#37392: Multisite "Sites" screen: Add links to filter websites by status
----------------------------------------+------------------------------
 Reporter:  thomaswm                    |       Owner:  mnelson4
     Type:  enhancement                 |      Status:  assigned
 Priority:  normal                      |   Milestone:  Awaiting Review
Component:  Networks and Sites          |     Version:
 Severity:  normal                      |  Resolution:
 Keywords:  has-patch needs-unit-tests  |     Focuses:  multisite
----------------------------------------+------------------------------

Comment (by flixos90):

 Thanks for the updated patch @mnelson4, sorry it took me so long to
 review.

 This looks great for the most part! A few observations:

 '''class-wp-ms-sites-list-table.php'''

 * `all` should probably not be part of the `$slug_to_label` array, as it's
 not a status and its link should have a different structure (it shouldn't
 do `?status=all`, but instead should not add any `status` query arg). See
 how https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes
 /class-wp-posts-list-table.php#L323 does that. The array should then also
 receive a more clear name such as `$statuses`, and the keys in the loop
 could be `$status` (`$this_status` looks weird as it can easily be
 confused with something like `$this->status`).

 '''ms-functions.php'''

 * In `wp_count_sites()`, try to be less strict about the provided value.
 `! $network_id` should be sufficient to fall back to
 `get_current_network_id()`. Before that you might wanna cast it to an
 integer.
 * I agree with your comment regarding the SQL query. Leave it as is in
 this patch.

 Please also have another look at spacing and blank lines inside of doc
 blocks. There are a number of missing blank lines there, and some parts
 are incorrectly indented.

 Regarding general approach: I think the approach we're taking is fine for
 now, but we should consider other approaches than how core has handled
 this previously too, just in case. Particularly, site statuses are
 different from post statuses because a site can have multiple statuses at
 the same time. The current approach doesn't, while maybe being sufficient,
 doesn't reflect that, for example it wouldn't allow to select all sites
 that are "archived and mature", or "deleted and not mature". That could
 only be covered by something like a `select multiple`, multiple checkboxes
 or dropdowns - again, not sure we should do this, but we should think
 about it further before proceeding with this just because core has done
 that before (in a slightly different scenario).

--
Ticket URL: <https://core.trac.wordpress.org/ticket/37392#comment:16>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform