#37684 closed enhancement (fixed)
Allow sites to have filterable "states" akin to posts & media
Reported by: | johnjamesjacoby | Owned by: | johnjamesjacoby |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | 3.0 |
Component: | Networks and Sites | Keywords: | has-patch has-dev-note |
Focuses: | multisite | Cc: |
Description (last modified by )
Posts & Media have "states". (See _media_states()
& _post_states()
for more.)
It would be nice if this paradigm existed for Sites, too, beyond just statuses. It would allow plugins to extend those states, for example, having "Protected" sites that cannot be deleted, even by super admins, or to communicate to network admins which site is the "Main" site in the network, which right now is not visually conveyed to the user.
Attachments (8)
Change History (44)
#1
@
8 years ago
Heh.
So... I'm a bit slow. Sites *do* have states, kinda, but they're baked into WP_MS_Sites_List_Table::column_blogname()
and not currently filterable. I'll update my patch to normalize this with Posts & Media.
@
8 years ago
An alternative approach that adds site_states()
as a method in WP_MS_Sites_List_Table
. This is cleaner, but makes the idea & output exclusive to this list table object, meaning plugins wanting to take advantage of site-states would need to duplicate this bit of code.
#2
@
8 years ago
- Description modified (diff)
- Summary changed from Allow sites to have "states" akin to posts & media to Allow sites to have filterable "states" akin to posts & media
Editing the title & description to focus on adding the filter & debating the correct approach going forward for improving site-states.
#4
@
8 years ago
As someone who often works on large multisite and multinetwork installs I think this is a great feature to have.
#7
@
8 years ago
- Keywords has-patch added; good-first-bug removed
Interesting enhancement, +1 on this.
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
7 years ago
#10
@
7 years ago
- Owner set to mnelson4
- Status changed from new to assigned
Per today's Slack chat, @mnelson4 is interested in working on this, particularly on filters for site states though (where the current patch does not implement filters, but displays each site's states in the list table).
I think it would make sense to implement the filters as a separate patch, completely independent from the existing ones. Maybe name the files something like 37684.filters.diff
to have a clear separation.
#11
@
7 years ago
Hey @flixos90 I think I'd actually like to keep this ticket's purpose as-is: just improving the "site states" and making that text filterable. I'd like to reopen #37392 and do the work for adding links to filter sites on that ticket. I think we both got a bit confused by this ticket mentioning "filterable states" (as in, having site states upon which apply_filters(...)
is called) and that other ticket mentioning "filter websites" (as in, narrow down the results to only include sites of a certani type).
Does that sound ok to you?
#12
@
7 years ago
@mnelson4 Good point, that makes complete sense.
I don't think I can unassign you from this one here unless I assign somebody else, so you're still gonna be the owner of this one for now. But don't worry about it. :)
#13
@
7 years ago
kk, I suppose these commits will need to be updated since #41057. If anyone gets a chance to do that, I'd suggest leaving a comment on this ticket saying so, in order to avoid duplicated effort
This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.
7 years ago
#15
@
7 years ago
Thanks @jeremyfelt, that latest patch applies cleanly. IMO https://core.trac.wordpress.org/attachment/ticket/37684/37684.03.diff sounds good because it sounds like this code is specific the Sites list table (other code elsewhere might want to know what states a blog is in, but want to format the HTML differently).
My ideal, though, would be to have a method on WP_Site
for getting an array of all the site's states; and then use that list from the sites list table.
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 pbiron. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
6 years ago
#19
@
6 years ago
- Keywords needs-refresh added
- Milestone changed from Awaiting Review to 5.3
- Owner changed from mnelson4 to johnjamesjacoby
- Version set to 3.0
Marked as 5.3. Assigned to @johnjamesjacoby to review and commit.
#20
@
6 years ago
I've been playing with 37684.2.diff a little bit and will have an updated patch for this early tomorrow.
#21
@
6 years ago
- Keywords needs-refresh removed
37684.3.diff is mostly the same as 37684.2.diff, but with the following differences:
- passes the current site object to the new
display_site_states
filter - corrects the
$site_states
index for theMain
state (it wasdeleted
in 37684.2.diff) - moves the call to the new
site_states()
method inside the<strong>...</strong>
HTML tag, to be consistent with other list tables that support states. - updates the
@since
tags to 5.3.0
#22
@
6 years ago
Another change that I think might be warranted would be to change
!#php foreach ( $this->status_list as $status => $col ) { if ( $_site->{$status} == 1 ) { $site_states[] = $col[1]; } }
to
!#php foreach ( $this->status_list as $status => $col ) { if ( $_site->{$status} == 1 ) { $site_states[ $col[0] ] = $col[1]; } }
That is, to give indexes to the states related to site status (e.g., span, deleted, etc).
#23
@
6 years ago
A couple other things I just came across while testing the patch on #37392:
WP_MS_Sites_List_Table::$status_list
does not contain 'Public', hence by default,public
sites don't get a "site_state". Of course, plugins could add that state with the newdisplay_site_states
filter, but I think it would be better handled in core by adding 'Public' to the$status_list
array.
- with the addition of "views" for site statuses in #37392, I think it makes sense to have
site_states()
exclude a given "site_state when that state is the same as the current "view", just as_post_states()
excludes Pending when the current "view" forWP_Posts_List_Table
is "Pending".
Any thoughts on those 2 points?
#24
follow-up:
↓ 25
@
6 years ago
- I think “Public” is assumed, much in the same way that “Published” is for posts, so it is correct not to include it.
- I agree that showing the state in each row for the current filter/view is redundant, and that not including it there is correct.
#25
in reply to:
↑ 24
;
follow-up:
↓ 30
@
6 years ago
Replying to johnjamesjacoby:
- I think “Public” is assumed, much in the same way that “Published” is for posts, so it is correct not to include it.
I don't feel strongly either way, but there is an arguably significant difference between post statuses and site statuses: post statuses are mutually exclusive (a post can't be both "Published" and "Pending") whereas site statuses aren't (a site can be both "Public" and "Spam").
ETA: #37683 comment:16 also mentions the differences between post and site statuses, when discussing the "approach" that the (then current) patch for #37684 takes.
- I agree that showing the state in each row for the current filter/view is redundant, and that not including it there is correct.
Thanx for confirmation on that. I'll wait a while for others to comment before submitting a revised patch with that behavior.
#26
@
6 years ago
+1 for this ticket, it will be a nice improvement.
The patch looks good so far, thanks for your work.
#27
@
5 years ago
5.3 beta 1 is due in 1 week (Sep 23). What needs to be done to get this committed?
This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.
5 years ago
#30
in reply to:
↑ 25
;
follow-up:
↓ 33
@
5 years ago
Replying to pbiron:
Replying to johnjamesjacoby:
- I agree that showing the state in each row for the current filter/view is redundant, and that not including it there is correct.
Thanx for confirmation on that. I'll wait a while for others to comment before submitting a revised patch with that behavior.
While working on the dev-note that will cover this ticket I realized that what got committed doesn't include that suggestion I made (my bad, I never added a patch that included it).
37684.4.diff remedies that.
Can we reopen this to get that new patch committed?
This ticket was mentioned in Slack in #core-multisite by pbiron. View the logs.
5 years ago
#33
in reply to:
↑ 30
;
follow-up:
↓ 34
@
5 years ago
Replying to pbiron:
Can we reopen this to get that new patch committed?
Before 37684.4.diff gets committed, I have a question.
@garrett-eclipse pointed out to me that:
- the conditional
if ( $_site->{$status} == 1 )
should use a Yoda condition
And while talking to him about that, I realized that that comparison should be strict if at all possible (according to WPCS).
As far as I can tell, $_site
is always a direct row from the blogs table and hence, the values in the various status properties will always be strings (either "0"
or "1"
). Can anyone confirm that?
Or was the comparison (introduced in 37684.03.diff and carried through all the subsequent patches) made loose intentionally because under some circumstances those properties might be ints?
For WPCS purposes, would it make sense to do:
if ( 1 === intval( $_sites->{$status} ) )
#34
in reply to:
↑ 33
@
5 years ago
- Keywords 2nd-opinion removed
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to pbiron:
As far as I can tell,
$_site
is always a direct row from the blogs table and hence, the values in the various status properties will always be strings (either"0"
or"1"
). Can anyone confirm that?
The WP_Site
class includes this in the various property documentations:
* A numeric string, for compatibility reasons.
Or was the comparison (introduced in 37684.03.diff and carried through all the subsequent patches) made loose intentionally because under some circumstances those properties might be ints?
I can't speak with authority on this, because while I prefer strict comparisons in my own code, there are reasons not to be strict in WordPress, and I consider this one of those cases.
For WPCS purposes, would it make sense to do:
if ( 1 === intval( $_sites->{$status} ) )
This seems fine. Another idea is to use absint()
on that status. It is derived from a tinyint
database column type, and absint()
would prevent cleverly disguising this with negative values via hooks.
I am comfortable committing your recommendation (not absint()
for now) and we can revisit further hardening anytime afterwards.
Add
_site_states()
towp-admin/includes/template.php
and use in Sites list-table