Opened 8 years ago
Last modified 8 years ago
#38071 new enhancement
Add status links to network/sites.php
Reported by: | 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)
Change History (26)
#1
follow-up:
↓ 2
@
8 years ago
@johnjamesjacoby Isn't this a duplicate of your own #37684? Or is there a difference I didn't get yet?
@
8 years ago
First pass, introduces new wp_count_sites()
function to ms-functions.php (akin to wp_count_posts()
)
#4
@
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
@
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:
↓ 13
↓ 14
@
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
@
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:
↓ 11
@
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
@
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 aswp_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
@
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
@
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'] ); }
#14
in reply to:
↑ 7
@
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.
#15
@
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.
#16
@
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()
.
#17
@
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
.
No links in sites.php