#37392 closed enhancement (fixed)
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: | has-patch has-unit-tests has-dev-note |
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 (16)
Change History (82)
#1
@
8 years ago
- Milestone Awaiting Review deleted
- Resolution set to duplicate
- Status changed from new to closed
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
7 years ago
#3
@
7 years 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.
#5
@
7 years 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
@
7 years 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.
#7
@
7 years 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 thatms-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 anstdClass
with the propertiesall_blogs
,public
,archived
,mature
,spam
anddeleted
. 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.
7 years ago
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
7 years ago
#10
@
7 years 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 ofesc_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 ofsprintf()
to see correct indentation in case of longer content. In the first case here that would mean you end up with ansprintf()
inside anothersprintf()
, 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 existingwp_count_posts()
for example. I think it should be part ofms-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 usesget_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 ofsites
now (that one houses all other site-related data), and the key could becounts-{$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. Seewp_count_posts()
for an example how it handles the caching. In addition, we'd need to clear the cache inclean_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 sayobject
instead ofstdClass
though.
General
- Very minor, but you can probably update the versions from
4.9.3
to5.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.
#12
@
7 years 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.
7 years ago
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
7 years ago
#16
@
7 years 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 anystatus
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 toget_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
@
7 years 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.
7 years ago
#19
@
7 years 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.
#20
@
7 years ago
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
#21
follow-up:
↓ 22
@
7 years 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
@
6 years 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 havingall
be in the array is avoids quite a bit of repeated code. If you still prefer not havingall
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.
@
6 years 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
@
6 years ago
Same as previous but I missed one line had spaces instead of tabs (IDE refuses to comply!)
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
6 years ago
#25
@
6 years 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.
6 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
5 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
#31
@
5 years 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
@
5 years 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?
#33
@
5 years 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
@
5 years 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
@
5 years 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()
hasFROM 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:
↓ 37
@
5 years 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
@
5 years 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.
#40
follow-up:
↓ 41
@
5 years ago
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.
#41
in reply to:
↑ 40
@
5 years ago
- Keywords needs-refresh removed
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.
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
5 years ago
#43
follow-up:
↓ 49
@
5 years ago
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):
- in the
foreach
loop,$_args[ $status ]
needs to be unset before the next iteration of the loop, otherwise the query for, e.g.,archived
becomespublic = 1 AND archived = 1
- even with number 1 fixed, reusing the
WP_Site_Query()
object seems to cause problems. Not sure if it's a bug inWP_Site_Query::query()
or whether it's just not intended to be reused like that. ButWP_Site_Query::$sql_clauses['where']
doesn't get reinitialized between repeated calls toWP_Site_Query::query()
, which results in the query forarchive = 1
to actually bepublic = 1 AND archived = 1
The effect of the above is that the counts are all wrong!
#44
@
5 years ago
@pbiron bummer. You’re right. My patch is not working correctly.
Disregard it, and iterate from @spacedmonkey’s.
#45
@
5 years ago
37392.2.diff is like 37392.9.patch, with the following differences:
wp_count_sites()
uses separateWP_Site_Query
objects for each status in the loop- removed
$wpdb
global that was unused - cleaned up the DocBlocks a little bit
This ticket was mentioned in Slack in #core by pbiron. View the logs.
5 years ago
#48
@
5 years ago
I took 37392.2.diff for a spin on a test site and it worked well for me. The counts were accurate.
#49
in reply to:
↑ 43
@
5 years ago
Replying to pbiron:
- even with number 1 fixed, reusing the
WP_Site_Query()
object seems to cause problems. Not sure if it's a bug inWP_Site_Query::query()
or whether it's just not intended to be reused like that. ButWP_Site_Query::$sql_clauses['where']
doesn't get reinitialized between repeated calls toWP_Site_Query::query()
, which results in the query forarchive = 1
to actually bepublic = 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).
#51
follow-up:
↓ 52
@
5 years ago
- Milestone changed from 5.3 to 5.4
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.
#52
in reply to:
↑ 51
@
5 years ago
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.
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 by jjj. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-committers by david.baumwald. View the logs.
5 years ago
#57
@
5 years ago
- Milestone changed from 5.4 to 5.3
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.
#62
@
5 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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
#63
@
5 years ago
- Resolution set to fixed
- Status changed from reopened to closed
The incorrect placeholders, missing context and missing translator comments are handled in #48210.
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.