WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 weeks ago

#37684 assigned enhancement

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: 2nd-opinion has-patch
Focuses: multisite Cc:

Description (last modified by 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.

Attachments (6)

37684.diff (2.2 KB) - added by johnjamesjacoby 3 years ago.
Add _site_states() to wp-admin/includes/template.php and use in Sites list-table
Screen Shot 2016-08-17 at 03.10.31.png (132.5 KB) - added by johnjamesjacoby 3 years ago.
Screenshot. See "-- Main" state in top row
37684.02.diff (4.2 KB) - added by johnjamesjacoby 3 years ago.
Remove site states from WP_MS_Sites_List_Table class
37684.03.diff (2.9 KB) - added by johnjamesjacoby 3 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.
37684.2.diff (2.8 KB) - added by jeremyfelt 17 months ago.
37684.3.diff (3.4 KB) - added by pbiron 3 weeks ago.
pass the current site to the 'display_site_states' filter.

Download all attachments as: .zip

Change History (32)

@johnjamesjacoby
3 years ago

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

@johnjamesjacoby
3 years ago

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

#1 @johnjamesjacoby
3 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.

@johnjamesjacoby
3 years ago

Remove site states from WP_MS_Sites_List_Table class

@johnjamesjacoby
3 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 @johnjamesjacoby
3 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.

#3 @johnjamesjacoby
3 years ago

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

#4 @blobaugh
3 years ago

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

#5 @thomaswm
3 years ago

Related: #37392

#6 @flixos90
3 years ago

#37392 was marked as a duplicate.

#7 @Mista-Flo
3 years ago

  • Keywords has-patch added; good-first-bug removed

Interesting enhancement, +1 on this.

#8 @mnelson4
22 months ago

+1 this, found ourselves wanting it today

Last edited 22 months ago by mnelson4 (previous) (diff)

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


19 months ago

#10 @flixos90
19 months 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 @mnelson4
19 months 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 @flixos90
19 months 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 @mnelson4
19 months 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

@jeremyfelt
17 months ago

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


17 months ago

#15 @mnelson4
17 months 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.


15 months ago

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


5 weeks ago

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


3 weeks ago

#19 @spacedmonkey
3 weeks 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 @pbiron
3 weeks ago

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

@pbiron
3 weeks ago

pass the current site to the 'display_site_states' filter.

#21 @pbiron
3 weeks ago

  • Keywords needs-refresh removed

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
Last edited 3 weeks ago by pbiron (previous) (diff)

#22 @pbiron
3 weeks 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 @pbiron
3 weeks ago

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

#24 follow-up: @johnjamesjacoby
3 weeks ago

  1. I think “Public” is assumed, much in the same way that “Published” is for posts, so it is correct not to include it.
  1. 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 @pbiron
3 weeks ago

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.

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

Last edited 3 weeks ago by pbiron (previous) (diff)

#26 @Mista-Flo
3 weeks ago

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

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

Note: See TracTickets for help on using tickets.