WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 5 weeks ago

Last modified 3 weeks ago

#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:
PR Number:

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 (8)

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 22 months ago.
37684.3.diff (3.4 KB) - added by pbiron 5 months ago.
pass the current site to the 'display_site_states' filter.
37684.4.diff (1.1 KB) - added by pbiron 6 weeks ago.
37684.5.diff (558 bytes) - added by johnjamesjacoby 5 weeks ago.
Strict comparison, Yoda condition, and avoid redundant State visibility

Download all attachments as: .zip

Change History (44)

@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
2 years ago

+1 this, found ourselves wanting it today

Last edited 2 years ago by mnelson4 (previous) (diff)

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


2 years ago

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

@jeremyfelt
22 months ago

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


22 months ago

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


20 months ago

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


6 months ago

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


5 months ago

#19 @spacedmonkey
5 months 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
5 months ago

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

@pbiron
5 months ago

pass the current site to the 'display_site_states' filter.

#21 @pbiron
5 months 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 5 months ago by pbiron (previous) (diff)

#22 @pbiron
5 months 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
5 months 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
5 months 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 ; follow-up: @pbiron
5 months 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 5 months ago by pbiron (previous) (diff)

#26 @Mista-Flo
5 months ago

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

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

#27 @pbiron
8 weeks ago

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

#28 @johnjamesjacoby
8 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

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


6 weeks ago

@pbiron
6 weeks ago

#30 in reply to: ↑ 25 ; follow-up: @pbiron
6 weeks ago

Replying to pbiron:

Replying to johnjamesjacoby:

  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.

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?

#31 @justinahinon
5 weeks ago

  • Keywords needs-dev-note added

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


5 weeks ago

#33 in reply to: ↑ 30 ; follow-up: @pbiron
5 weeks 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 @johnjamesjacoby
5 weeks 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.

@johnjamesjacoby
5 weeks ago

Strict comparison, Yoda condition, and avoid redundant State visibility

#35 @johnjamesjacoby
5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.