Make WordPress Core

Opened 8 years ago

Last modified 8 years ago

#38071 new enhancement

Add status links to network/sites.php

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: has-patch has-screenshots has-unit-tests
Focuses: multisite Cc:

Description

Although sites have a "status" property, there is no interface for identifying what sites have which statuses, or their associated counts.

Screens that do have this:

  • Posts
  • Pages
  • Users
  • Comments
  • Plugins
  • Themes (network or list view)

Attachments (9)

Screen Shot 2016-09-15 at 2.04.36 PM.png (24.2 KB) - added by johnjamesjacoby 8 years ago.
No links in sites.php
Screen Shot 2016-09-15 at 2.04.26 PM.png (29.4 KB) - added by johnjamesjacoby 8 years ago.
Links in users.php
Screen Shot 2016-09-15 at 2.08.20 PM.png (45.7 KB) - added by johnjamesjacoby 8 years ago.
Links in edit.php
38071.patch (7.0 KB) - added by johnjamesjacoby 8 years ago.
First pass, introduces new wp_count_sites() function to ms-functions.php (akin to wp_count_posts())
Screen Shot 2016-09-15 at 3.48.26 PM.png (123.9 KB) - added by johnjamesjacoby 8 years ago.
38071.1.diff (7.5 KB) - added by pbiron 8 years ago.
here's my patch, which is against the current core code
38071.2.diff (7.4 KB) - added by pbiron 8 years ago.
countSites.php (7.5 KB) - added by pbiron 8 years ago.
wpMSSitesListTable-38071.php (9.0 KB) - added by pbiron 8 years ago.

Download all attachments as: .zip

Change History (26)

@johnjamesjacoby
8 years ago

No links in sites.php

#1 follow-up: @flixos90
8 years ago

@johnjamesjacoby Isn't this a duplicate of your own #37684? Or is there a difference I didn't get yet?

#2 in reply to: ↑ 1 @johnjamesjacoby
8 years ago

Replying to flixos90:

@johnjamesjacoby Isn't this a duplicate of your own #37684? Or is there a difference I didn't get yet?

That ticket is for inline states, in each table row.

@johnjamesjacoby
8 years ago

First pass, introduces new wp_count_sites() function to ms-functions.php (akin to wp_count_posts())

#3 @johnjamesjacoby
8 years ago

  • Keywords has-patch has-screenshots added

#4 @jeremyfelt
8 years ago

  • Keywords needs-unit-tests added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.7

I like the idea of having status links for sites. I didn't spend a lot of time with wp_count_sites(), but we should probably get some tests written for it. We'll also need to make sure the count cache is cleared when sites are created, updated with another status, or deleted.

#5 @jorbin
8 years ago

If this is going to make it into 4.7, it needs the unit tests that @jeremyfelt has requested.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


8 years ago

#7 follow-ups: @rheinardkorf
8 years ago

Definitely needs unit tests.

The following is not a reliable method.

protected function is_base_request() {
		$vars = $_GET;
		unset( $vars['paged'] );

		if ( empty( $vars ) ) {
			return true;
		}

		return 1 === count( $vars ) && ! empty( $vars['mode'] );
	}

Plugins may add additional functionality via URI, so simply unsetting 'paged' or checking for only 'mode' is does not give the intended result. It is better to check for the presence/absence of 'site_status'.

The rest is looking pretty good.

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


8 years ago

#9 @PieWP
8 years ago

In regards to the wp_count_sites, why use the foreach to sum up the status instead of an aggregated query? Might cause issues when someone has many subsites within their network.

"SELECT 
     SUM(public) AS `public`, 
     SUM(archived) AS `archived`, 
     SUM(mature) AS `mature`, 
     SUM(spam) AS `spam`, 
     SUM(deleted) as `deleted` 
FROM {$wpdb->blogs} WHERE site_id = %d"

#10 follow-up: @flixos90
8 years ago

  • Milestone changed from 4.7 to Future Release

We're too late in the release cycle to get this into 4.7 as it needs further though on the approach. Let's continue to work on it so that we can hopefully put it into 4.8.

Regarding the details, I agree with the previous comments. I also would like to investigate ways to enhance the existing get_blog_count() function for that usage as wp_count_sites(), while it works completely different in the patch implementation, sounds like it does the same. Related: #37865 (and then we could add other parameters to it and enhance how it works)

#11 in reply to: ↑ 10 @rheinardkorf
8 years ago

Replying to flixos90:

We're too late in the release cycle to get this into 4.7 as it needs further though on the approach. Let's continue to work on it so that we can hopefully put it into 4.8.

Regarding the details, I agree with the previous comments. I also would like to investigate ways to enhance the existing get_blog_count() function for that usage as wp_count_sites(), while it works completely different in the patch implementation, sounds like it does the same. Related: #37865 (and then we could add other parameters to it and enhance how it works)

Agreed. If we're going to make better use of site statuses it would also make sense to add other enhancements. E.g. filtering the statuses so that it can be extended.

#12 @pbiron
8 years ago

Guess I should have seached Trac before spending several hours today implementing almost exactly what @johnjamesjacoby did...it was a good learning experience for me, nonetheless :-)

#13 in reply to: ↑ 7 @pbiron
8 years ago

Replying to rheinardkorf:

Definitely needs unit tests.

The following is not a reliable method.

protected function is_base_request() {
		$vars = $_GET;
		unset( $vars['paged'] );

		if ( empty( $vars ) ) {
			return true;
		}

		return 1 === count( $vars ) && ! empty( $vars['mode'] );
	}

Plugins may add additional functionality via URI, so simply unsetting 'paged' or checking for only 'mode' is does not give the intended result. It is better to check for the presence/absence of 'site_status'.

The rest is looking pretty good.

This is what I came up with for is_base_request() which I think is more reliable:

protected function is_base_request() {
	$vars = $_GET;
	unset( $vars['paged'] );

	if ( empty( $vars ) ) {
		return true;
	} elseif ( 1 === count( $vars ) && ! empty( $vars['site_status'] ) ) {
		return false;
	}

	return 1 === count( $vars ) && ! empty( $vars['mode'] );
}
Last edited 8 years ago by pbiron (previous) (diff)

#14 in reply to: ↑ 7 @pbiron
8 years ago

Replying to rheinardkorf:

Definitely needs unit tests.

How does on go about writing unit tests for proposed patches?

I'd like to see this idea go forward, so I'd be willing to take a stab at writing unit tests if someone can point me to the details.

@pbiron
8 years ago

here's my patch, which is against the current core code

#15 @pbiron
8 years ago

The patch I just attached is against the current core code, which has changed a bit since @johnjamesjacoby posted his patch.

I also borrowed @PieWP's idea for the implementation of wp_count_sites() and slightly modified the implementation of is_base_request() I commented on above.

Again, if someone can point me to the rules for writing unit tests for WP, I'll take a stab at that over the next few days...and maybe we can get this 4.8.

Version 0, edited 8 years ago by pbiron (next)

@pbiron
8 years ago

@pbiron
8 years ago

#16 @pbiron
8 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

The patch I just added has minor updates to the inline docs, adds a 'site_status' hidden field to the search form (so that searches are limited to the currently selected site_status as with posts) and simplifies WP_MS_Sites_List_Table::is_base_request().

Last edited 8 years ago by pbiron (previous) (diff)

#17 @pbiron
8 years ago

I found enough info about writing unit tests for the WP test-suite to take a stab at writing tests for this ticket.

Since these are the 1st tests I've written against this test-suite I highly suggest that someone double-check them to make sure they are actually testing what I think they are testing) and the setup and teardown (to make sure they don't adversely affect other tests.

I separated the tests for wp_site_counts() and those for sites.php into 2 separate files. As noted in a comment in wpMSSitesListTable-38071.php, once this ticket is accepted, those tests should be integrated into wpMSSitesListTable.php...I figured it would be easier to verify that my tests were correct if they were isolated from the existing tests in wpMSSitesListTable.php.

Note: See TracTickets for help on using tickets.