WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 10 days ago

#37392 assigned enhancement

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

Reported by: thomaswm Owned by: mnelson4
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: needs-unit-tests needs-patch needs-refresh
Focuses: multisite Cc:

Description

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.

Attachments (12)

37392.patch (4.5 KB) - added by mnelson4 17 months ago.
1st implementation of this feature in a git patch
37392.2.patch (5.3 KB) - added by mnelson4 16 months ago.
First Revision of changes
37392.3.patch (5.3 KB) - added by mnelson4 16 months ago.
same as previous patch, but the last one had some spaces instead of tabs
37392.4.patch (4.9 KB) - added by mnelson4 16 months ago.
Same as previous, except no unnecessary removal of line in ms-functions.php
37392.5.patch (4.4 KB) - added by mnelson4 15 months ago.
Patch uploaded in response to https://core.trac.wordpress.org/ticket/37392#comment:16
37392.6.patch (4.9 KB) - added by mnelson4 11 months ago.
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
37392.7.patch (4.9 KB) - added by mnelson4 11 months ago.
Same as previous but I missed one line had spaces instead of tabs (IDE refuses to comply!)
37392.8.patch (4.9 KB) - added by mnelson4 11 months ago.
Same as previous but there were extra sapces between $counts and wp_count_sites()
all public.png (50.2 KB) - added by mnelson4 10 months ago.
Sites list table when all sites are public (none are archived or mature etc)
unfiltered with various stati.png (39.2 KB) - added by mnelson4 10 months ago.
Sites list table with archived, mature, and "deleted" sites
filtered.png (34.0 KB) - added by mnelson4 10 months ago.
Sites list table filtering to only see "archived"
37392.diff (4.4 KB) - added by spacedmonkey 11 days ago.

Download all attachments as: .zip

Change History (49)

#1 @flixos90
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

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.

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


19 months ago

#3 @flixos90
19 months ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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.

#4 @flixos90
19 months ago

  • Owner set to mnelson4
  • Status changed from reopened to assigned

#5 @mnelson4
19 months ago

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?

#6 @flixos90
18 months ago

@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
17 months ago

1st implementation of this feature in a git patch

#7 @mnelson4
17 months ago

  • Keywords has-patch needs-unit-tests 2nd-opinion added

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.

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


17 months ago

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


17 months ago

#10 @flixos90
17 months ago

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.

#11 @mnelson4
17 months ago

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

@mnelson4
16 months ago

First Revision of changes

@mnelson4
16 months ago

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

@mnelson4
16 months ago

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

#12 @mnelson4
16 months ago

  • Keywords 2nd-opinion removed

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?

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


16 months ago

#14 @SergeyBiryukov
16 months ago

  • Milestone set to Awaiting Review

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


16 months ago

#16 @flixos90
15 months ago

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

#17 @mnelson4
15 months ago

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?

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


15 months ago

#19 @mnelson4
15 months ago

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.

#21 follow-up: @mnelson4
15 months ago

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

#22 in reply to: ↑ 21 @flixos90
13 months ago

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
11 months ago

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
11 months ago

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

@mnelson4
11 months ago

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

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


10 months ago

#24 @johnjamesjacoby
10 months ago

  • Keywords needs-screenshots added

@mnelson4
10 months ago

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

@mnelson4
10 months ago

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

@mnelson4
10 months ago

Sites list table filtering to only see "archived"

#25 @mnelson4
10 months ago

  • Keywords needs-screenshots removed

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

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


9 months ago

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


9 months ago

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


7 weeks ago

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


4 weeks ago

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


11 days ago

#31 @spacedmonkey
11 days ago

  • Keywords needs-patch needs-refresh added; has-patch removed
  • Milestone changed from Awaiting Review to 5.3
  • Version set to 3.0

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.

#32 @mnelson4
11 days ago

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
11 days ago

#33 @spacedmonkey
11 days ago

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.

#34 @pbiron
10 days ago

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.

#35 @pbiron
10 days ago

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

#36 follow-up: @mnelson4
10 days ago

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

#37 in reply to: ↑ 36 @pbiron
10 days ago

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.

Note: See TracTickets for help on using tickets.