Allow sites to have filterable "states" akin to posts & media

[johnjamesjacoby]

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.

[johnjamesjacoby]

Add _site_states() to wp-admin/includes/template.php and use in Sites list-table

[johnjamesjacoby]

Screenshot. See "-- Main" state in top row

[johnjamesjacoby]

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.

[johnjamesjacoby]

Remove site states from WP_MS_Sites_List_Table class

[johnjamesjacoby]

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.

[johnjamesjacoby]

Editing the title & description to focus on adding the filter & debating the correct approach going forward for improving site-states.

[johnjamesjacoby]

Paging @jeremyfelt for a 2nd-opinion & sign-off.

[blobaugh]

As someone who often works on large multisite and multinetwork installs I think this is a great feature to have.

[thomaswm]

Related: #37392

[flixos90]

#37392 was marked as a duplicate.

[Mista-Flo]

Interesting enhancement, +1 on this.

[mnelson4]

+1 this, found ourselves wanting it today

[flixos90]

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.

[mnelson4]

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?

[flixos90]

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

[mnelson4]

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

[mnelson4]

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.

[spacedmonkey]

Marked as 5.3. Assigned to @johnjamesjacoby to review and commit.

[pbiron]

I've been playing with 37684.2.diff a little bit and will have an updated patch for this early tomorrow.

[pbiron]

pass the current site to the 'display_site_states' filter.

[pbiron]

37684.3.diff is mostly the same as 37684.2.diff, but with the following differences:

1. passes the current site object to the new display_site_states filter
2. corrects the $site_states index for the Main state (it was deleted in 37684.2.diff)
3. 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.
4. updates the @since tags to 5.3.0

[pbiron]

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

[pbiron]

A couple other things I just came across while testing the patch on #37392:

1. 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 new display_site_states filter, but I think it would be better handled in core by adding 'Public' to the $status_list array.

2. 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" for WP_Posts_List_Table is "Pending".

Any thoughts on those 2 points?

[johnjamesjacoby]

1. I think “Public” is assumed, much in the same way that “Published” is for posts, so it is correct not to include it.

2. I agree that showing the state in each row for the current filter/view is redundant, and that not including it there is correct.

[pbiron]

Replying to johnjamesjacoby:

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

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

[Mista-Flo]

+1 for this ticket, it will be a nice improvement.

The patch looks good so far, thanks for your work.

[pbiron]

5.3 beta 1 is due in 1 week (Sep 23). What needs to be done to get this committed?

[johnjamesjacoby]

In 46153:

Network Admin: Allow Sites to have filterable States in List Table rows.

This change introduces a new site_states() method to the Sites List Table class (with a new display_site_states filter inside of it) following the pattern popularized in other List Table classes before it (Posts, Media, etc...)

Fixes #37684. Props mnelson4, pbiron, jeremyfelt, johnjamesjacoby.

[pbiron]

Replying to pbiron:

Replying to johnjamesjacoby:

2. 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?

[pbiron]

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

[johnjamesjacoby]

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.

[johnjamesjacoby]

Strict comparison, Yoda condition, and avoid redundant State visibility

[johnjamesjacoby]

In 46441:

Network Admin: Improve Site States in List Table rows.

This commit switches a comparison to a Yoda condition, performs a more strict intval() check on the value of the Site Status column, and prevents a specific Site State from appearing in List Table rows when filtering by that same Site State already (to match the behavior of other List Table State implementations.)

Fixes #37684. Props pbiron.

[desrosj]

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