Multisite "Sites" screen: Add links to filter websites by status

[thomaswm]

I'd like to see a list of all archived sites in my multisite network. Currently, I have to go through all sites on the "Sites" screen. There's no possibility to filter them by status.

My suggestion is to add a list of links ("Archived", "Deativated", "Spam" etc.) to the top of the "Sites" screen. Clicking one of them would result in only archived/deleted/spam websites being displayed.

Most other screens in wp-admin have those links, for example to filter posts by post status.

[flixos90]

Thanks for the ticket @thomaswm!

Although yours has been around a little longer, there's #37684 now which has seen a little more progress lately, so let's continue the discussion there.

[flixos90]

Per https://core.trac.wordpress.org/ticket/37684#comment:11, this is actually not a duplicate, only a related ticket. This one here is about adding filters for site states, while the other one deals with display site states in the list table.

[mnelson4]

I saw a post somewhere (sorry I can't find it, I thought it was on make.wordpress.org) mentioning how we're going to want to add sites REST API endpoints, and use them from the admin. If that's the case, it would probably be a waste to try adding this in the old style, because we'll be wanting to rewrite it anyways right away. Right?

[flixos90]

@mnelson4 It's true that parts of the admin will be rewritten once the multisite API endpoints are in place, however this will be happening over the course of time, and also reasonably. If a change doesn't really improve anything other than using AJAX and the REST API, there's not much of a point.

I think this particular problem here would benefit from a solution based on current circumstances, as there are more pressing issues in multisite UX when it comes to using the REST API that would need to be handled first.

[mnelson4]

1st implementation of this feature in a git patch

[mnelson4]

Ok I added a patch that adds this feature. I based it off the users list table.

Some issues I wasn't sure about:

• what would be the best name for the new function, which for now is just called wp_count_sites(). Also just took a guess that ms-functions.php would be a good place for it
• the translatable strings which I basically copied from class-wp-users-list-table.php has HTML in it (I thought its best to have placeholders, because we don't want to burden translators with understanding HTML, and we certainly don't want translators to translate the HTML or the classes etc!). Eg __( '%1$s <span class="count">(%2$s)</span>' ) is one. I just left the HTML in there for now, but it seems it would be best to put placeholders in there instead.... although that would leave the string as just "%1$s %2$s(%3$s)%4$s" which is so vague that maybe we should just NOT translate it at all. Thoughts?
• I don't totally understand what the "public" flag on a blog row means. When I set that to false for a blog, I could still visit it on the front-end (even when not logged in), and its mostly absent from the UI (except if you click to "edit" a page). Perhaps we should exclude that view?
• wp_count_sites() returns an stdClass with the properties all_blogs, public, archived, mature, spam and deleted. Do we like those names? Would it be better as an array?

Lastly, I should probably add a unit test or two for wp_count_sites(), but I'd rather do that once I have confirmation from someone that that functionality looks good.

[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.

[mnelson4]

Ok thanks for the great feedback Felix, I'll make a new patch

[mnelson4]

First Revision of changes

[mnelson4]

same as previous patch, but the last one had some spaces instead of tabs

[mnelson4]

Same as previous, except no unnecessary removal of line in ms-functions.php

[mnelson4]

Ok I think the latest patch applies your suggestions @flixos90 (the previous ones did too, but had formatting mistakes).

The only one I didn't apply was

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()).

That could be good, but it would have beena bit ugly because the first selection (where it selects "all") would be an exception; I don't think we'll be adding new statuses soon; and IMO leaving it as simple SQL makes it easier to immediately understand what it's doing. However, if you feel strongly about it, I'm happy to add the loop like you suggested.

Let me know if I missed something!

If this looks good, what else is needed to get this ready?

[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).

[mnelson4]

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

No worries, thanks for the review!

we should consider other approaches than how core has handled this previously too... like a select multiple, multiple checkboxes

In my usage of multisite, it's rare for a site to have multiple statuses (actually, we mostly just mark sites as spam, and don't really use the other statuses much). So I wouldn't prioritize this, but if you think it's important we can attempt it.

In order to have parity with what WP_Site_Query can handle, where you can provide any of the statuses and a value of either true or false (and not including the status means "whatever"), I think we would need to list each status, and have a dropdown for it with options like "Yes", "No", or "Either", and have a button saying "Filter" or "Search".

To query for "archived and mature", the would set archived to "Yes", and mature to "Yes", and leave all the others at "Either". For "deleted and not mature" they would set deleted to "Yes", mature to "No", and leave all others on "Either". Thoughts?

Also, it's maybe this patch should just add actions and filters to WP core, and then create a plugin for this instead. Then we can see what feedback there is on that plugin, before committing to a decision for WP core. Thoughts?

[mnelson4]

We discussed this in slack, and we're going to stick with this UI initially (ie, we won't initially support filtering by multiples statuses).

I'll add some screenshots of the current UI, and work on the code tweaks @flixos90 suggested.

I think @flixos90 and @jeremyfelt will discuss where best to cache the query, but for now I'm going to leave it where it is.

[mnelson4]

Here's a screenshot of what this patch adds:
​https://drive.google.com/a/eventespresso.com/file/d/1GKiN9SNXs0aU4hEXcqsWVX9MWOFw8fev/view?usp=drivesdk

And here's a video: ​https://drive.google.com/file/d/1F8djO1CnkU21MMlmwPqNESNdbASAQds1/view

[mnelson4]

Patch uploaded in response to https://core.trac.wordpress.org/ticket/37392#comment:16

[mnelson4]

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.

I hear you that there shouldn't be a link with ?status=all, so I made an adjustment that avoids that, but I do prefer how having all be in the array is avoids quite a bit of repeated code. If you still prefer not having all in the array, I can make another patch that does that.

here are a number of missing blank lines there

I'm not sure where there should be blank lines. I double-checked the WP coding standards and don't see anything about where there should be blank lines. Could you point me to some documentation or help me see where blank lines are needed?

some parts are incorrectly indented.

I double-checked that it's using real tabs instead of spaces for indentation and don't see where the indentation is wrong, sorry!

I hope this is a bit better though

[flixos90]

Replying to mnelson4:

I hear you that there shouldn't be a link with ?status=all, so I made an adjustment that avoids that, but I do prefer how having all be in the array is avoids quite a bit of repeated code. If you still prefer not having all in the array, I can make another patch that does that.

That's fine with me. One minor detail, please always use strict comparisons, there's a non-strict comparison in line 232.

I'm not sure where there should be blank lines. I double-checked the WP coding standards and don't see anything about where there should be blank lines. Could you point me to some documentation or help me see where blank lines are needed?

Between docblock descriptions, since descriptions, param/return descriptions there should always be a blank line (blank meaning of course still put the asterisk there, but only that). For example between lines 207, 208 and 209 in class-wp-ms-sites-list-table.php or lines 1544 and 1545 in ms-blogs.php.

I double-checked that it's using real tabs instead of spaces for indentation and don't see where the indentation is wrong, sorry!

For example line 1545 of ms-blogs.php there are several extra spaces between the variable name and description.

[mnelson4]

Same as 37392.5 but fixed some spaces-intead-of-tabs, missing periods at the end of comments, and missing spaces in PHPdocs. Added a few empty lines in spaces too like I saw in some other functions

[mnelson4]

Same as previous but I missed one line had spaces instead of tabs (IDE refuses to comply!)

[mnelson4]

Same as previous but there were extra sapces between $counts and wp_count_sites()

[mnelson4]

Sites list table when all sites are public (none are archived or mature etc)

[mnelson4]

Sites list table with archived, mature, and "deleted" sites

[mnelson4]

Sites list table filtering to only see "archived"

[mnelson4]

Oh I suppose I should also add some unit tests for this eh?

[spacedmonkey]

The lastest patch needs a refresh. Personally I am not a fan of the wp_count_sites function, is has an uncached mysql query. I believe you could use WP_Site_Query to get counts of site, that would be cached and filtered.

Making as 5.3, in hopes we can get this in.

[mnelson4]

Thanks for giving it a look @spacedmonkey.
I’ll refresh the patch ASAP.
Did you look at https://core.trac.wordpress.org/attachment/ticket/37392/37392.8.patch ? In it, wp_count_sites does cache its result. Also, because it’s just getting a count, I think using WP_Site_Query would be very inefficient, because it gets the rows instead of just a count. What do you think?

[spacedmonkey]

I have taken a bash at refreshing the patch. See 37392.diff.

wp_count_sites did have caching but not cache invalidation. So I have converted it to use WP_Site_Query. In the original patch, it used the weird nested count queries found in, ​count_users. That query in count_users is a nightmare and doesn't scale. See #38741 for more information. After working on that ticket, it easier to do 6 small queries that one big query to get the data we need.

[pbiron]

I've given 37392.diff some testing.

It works like a charm!

I am definitely NOT your go-to guy on efficiency issues, but I will note that the version of wp_count_sites() is an order of magnitude SLOWER than that in 37392.8.patch...on a real-world network with ~2000 sites

• 37392.diff is averaging 0.03 seconds
• 37392.8.patch is averaging 0.002 seconds

Note: The above timings are on my local dev machine, which has a slow hard drive.

I can't comment on how important the cache invalidation that WP_Site_Query() provides would be in real-world applications.

[pbiron]

a couple of other minor comments about both 37392.diff and 37392.8.patch...

37392.diff

• there are spaces (instead of tab) in the DocBlock on get_views()

37392.8.patch

• the query in wp_count_sites() has FROM wp_blogs hardcoded and needs to use $wpdb->blogs

Both

• the inline comment in prepare_items() should say something like "Take into account the site status the user has selected." instead of "Take into account the role the user has selected."

[mnelson4]

Thanks for the patch @spacedmonkey and testing @pbiron !

wp_count_sites did have caching but not cache invalidation

Oh right! So I suppose if we used 37392.8.patch's wp_count_sites, we'd need to invalidate the cache every time a site was created, deleted, or its status was changed, right? That sounds like a bit of work...

That query in count_users is a nightmare and doesn't scale. See #38741 for more information.

You're right that query was based off count_users, although I suspect it would scale better than count_users because it doesn't use LIKE in the query.

I am definitely NOT your go-to guy on efficiency issues, but I will note that the version of wp_count_sites() is an order of magnitude SLOWER than that in 37392.8.patch...on a real-world network with ~2000 sites

I'm also no performance expert, but could also do some benchmarking if that would help.

But regardless, does 37392.diff sounds like it will be fast enough? Seeing how it avoids needing to do cache invalidation, it sounds like a winner to me (with the tweaks @pbiron mentioned).

[pbiron]

Replying to mnelson4:

But regardless, does 37392.diff sounds like it will be fast enough? Seeing how it avoids needing to do cache invalidation, it sounds like a winner to me.

I think so...I just wanted to put some numbers to the 2 in case no one else had.

[pbiron]

5.3 beta 1 is due in 1 week (Sep 23). What do we need to get this committed?

[johnjamesjacoby]

37392.9.patch is the same as 37392.diff with the following changes:

• Cleaned up some indentations that had both spaces and tabs 😔
• Cleaned up copy-pasta that referred to "role" instead of "status"
• Cleaned up wp_count_sites() by reordering some invocations, and limitingWP_Site_Query() object creation down to 1 object instead of one for every status

I think this is great, and a future iteration should not have the statuses hardcoded. See #12706.

[pbiron]

Replying to johnjamesjacoby:

37392.9.patch is the same as 37392.diff with the following changes:

Thanx for the updated patch. I'll give this a test later today (tomorrow at the latest).

Do we need unit tests? this ticket has the needs-unit-tests keyword.

I think this is great, and a future iteration should not have the statuses hardcoded. See #12706.

Agreed that that would a good future iteration...no sense in trying to jam it in at this late date.

[johnjamesjacoby]

Same as 37392.diff with changes to be explained in comment imminently

[pbiron]

The latest update to 37392.9.pacth now applies cleanly.

However, there seem to be a couple of unrelated problems with wp_count_sites() (that both result in the same problem):

1. in the foreach loop, $_args[ $status ] needs to be unset before the next iteration of the loop, otherwise the query for, e.g., archived becomes public = 1 AND archived = 1

2. even with number 1 fixed, reusing the WP_Site_Query() object seems to cause problems. Not sure if it's a bug in WP_Site_Query::query() or whether it's just not intended to be reused like that. But WP_Site_Query::$sql_clauses['where'] doesn't get reinitialized between repeated calls to WP_Site_Query::query(), which results in the query for archive = 1 to actually be public = 1 AND archived = 1

The effect of the above is that the counts are all wrong!

[johnjamesjacoby]

@pbiron bummer. You’re right. My patch is not working correctly.

Disregard it, and iterate from @spacedmonkey’s.

[pbiron]

37392.2.diff is like 37392.9.patch, with the following differences:

1. wp_count_sites() uses separate WP_Site_Query objects for each status in the loop
2. removed $wpdb global that was unused
3. cleaned up the DocBlocks a little bit

[mnelson4]

I took 37392.2.diff for a spin on a test site and it worked well for me. The counts were accurate.

[pbiron]

Replying to pbiron:

2. even with number 1 fixed, reusing the WP_Site_Query() object seems to cause problems. Not sure if it's a bug in WP_Site_Query::query() or whether it's just not intended to be reused like that. But WP_Site_Query::$sql_clauses['where'] doesn't get reinitialized between repeated calls to WP_Site_Query::query(), which results in the query for archive = 1 to actually be public = 1 AND archived = 1

Related: #48078

I opened another ticket to report the problem with calling WP_Site_Query::query() in a loop. Turns out that a couple of the other WP_XXX_Query classes have the same problem (and a couple don't).

[davidbaumwald]

This ticket is marked needs-unit-tests but still needs them. With version 5.3 Beta 1 landing today, this is being punted. Moving to 5.4 based on recent momentum.

[pbiron]

Replying to davidbaumwald:

This ticket is marked needs-unit-tests but still needs them. With version 5.3 Beta 1 landing today, this is being punted. Moving to 5.4 based on recent momentum.

@davidbaumwald I was just in the process of adding a unit test when I noticed you punted this ticket.

If anyone is available to test the updated patch (including the unit test), is it still possible to get this committed before beta 1 goes out today? I know time is tight.

[johnjamesjacoby]

Setting milestone back to 5.3 per Slack convo with @pbiron and @davidbaumwald.

Updated patch including unit tests makes this a go. I've reviewed, and will be committing imminently.

[davidbaumwald]

Thanks @jjj and @pbiron for getting this in at the last minute!

[johnjamesjacoby]

In 46251:

Multisite/Sites: Add links to filter websites by status.

This commit brings the Network-Admin Sites list page up-to-speed with other similar list-table powered pages, by adding links to filter the results by Site Status.

Includes a single unit test for the newly introduced wp_count_sites() multisite function, named to match the wp_count_ function pattern from other list tables.

Fixes #37392. Props mnelson4, spacedmonkey, pbiron.

[johnjamesjacoby]

In 46254:

Multisite/Sites: supplemental commit to r46251.

This commit adds the links to the list-table class itself (that were missed in r46251.)

See #37392. Props pbiron, thomaswm.

[desrosj]

In 46261:

Coding Standards: Fix a coding standards issue introduced in [46254].

See #37392.

[garrett-eclipse]

Hello, sorry to re-open this, let me know if a new ticket is better.

While translating the new strings I found there's a mix of placeholders in the $statuses array here;
​https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/includes/class-wp-ms-sites-list-table.php#L221-L228
Specifically the singular for All and Public use the %s placeholder while all others use %1$s.

This was introduced in Changeset#46254

From the docs on _n_noop I believe all of the placeholders there should use %s notation.
​https://developer.wordpress.org/reference/functions/_n_noop/
​https://codex.wordpress.org/Function_Reference/_n_noop

[garrett-eclipse]

Patch to address the _n_noop placeholders

[ocean90]

The incorrect placeholders, missing context and missing translator comments are handled in #48210.

[desrosj]

Dev note: ​https://make.wordpress.org/core/2019/10/15/enhancements-to-the-network-sites-screen-in-wordpress-5-3/