[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:
Component:  Networks and Sites                   |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch needs-unit-tests 2nd-      |     Focuses:  multisite
  opinion                                        |
-------------------------------------------------+-------------------------

Comment (by flixos90):

 Thanks for the patch @mnelson4, generally looking good! A few observations
 here:

 **class-wp-ms-sites-list-table.php**
 * Typo `deletected` in line 69
 * The if clause in lines 62 to 73 can all go into one line, per coding
 standards. The list of statuses is short enough to be on a single line.
 * The list of statuses in lines 225 to 229 should use `__()` instead of
 `esc_html__()` --> always escape as late as possible (usually when
 outputting); so here that should happen in lines 251 and 259.
 * Blank line 222 at method beginning is not needed, instead a few more
 blank lines ''inside'' the method code would help with readability. Also
 look out for having = signs similarly indented if there isn't a blank line
 between them.
 * Instead of concatenating the long strings from line 235-245 and 254-260,
 rather use `sprintf()`, like `<a href="%1$s"%2$s>%3$s</a>` (make sure to
 not put additional spaces where not needed). You can look at other core
 uses of `sprintf()` to see correct indentation in case of longer content.
 In the first case here that would mean you end up with an `sprintf()`
 inside another `sprintf()`, so in that case, put the inner one first and
 store it in a variable, to not overcomplicate readability there.
 * Instead of having a single label for each site status, rather following
 the pattern similar like post statuses do (see
 https://core.trac.wordpress.org/browser/trunk/src/wp-
 includes/post.php#L234 and
 https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-
 wp-posts-list-table.php#L356 for how it works there).

 **ms-functions.php**
 * I agree with the name `wp_count_sites()`, as it matches the existing
 `wp_count_posts()` for example. I think it should be part of `ms-
 blogs.php` though since that one houses the basic site API functions
 related to querying.
 * The query in `wp_count_sites()` currently doesn't take the network into
 account. It should only count sites of the current network. Even better
 would be: Add an optional `$network_id` parameter that uses
 `get_current_network_id()` if nothing provided, and use that then.
 * The query should definitely be cached. It's kind of tricky to find a
 suitable cache group and key here. Generally, those things are stored in
 the `counts` group, but that group is per site, and the sites count is
 global, so we can't use that. I'd suggest going with a group of `sites`
 now (that one houses all other site-related data), and the key could be
 `counts-{$network_id}`. @jeremyfelt What do you think about naming here?
 Which cache key and group would be suitable? Anyway, you can probably just
 use the above for now. See `wp_count_posts()` for an example how it
 handles the caching. In addition, we'd need to clear the cache in
 `clean_blog_cache()` for the network the site is part of.
 * Maybe it would make sense to build the SQL string by iterating through
 the different site statuses (you could put those in an array, just like in
 your `get_views()`).
 * The return value of an object is fine as it matches `wp_count_posts()`.
 In the docblock, rather say `object` instead of `stdClass` though.

 **General**
 * Very minor, but you can probably update the versions from `4.9.3` to
 `5.0`, as this will likely not go in as part of a minor release.
 * Check spacing and blank lines for docblocks and make sure all docblocks
 have a description.

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