Opened 9 years ago
Closed 9 years ago
#36675 closed task (blessed) (fixed)
WP_MS_Sites_List_Table should use WP_Site_Query
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.6 | Priority: | normal |
Severity: | normal | Version: | 4.6 |
Component: | Networks and Sites | Keywords: | has-patch has-unit-tests needs-docs |
Focuses: | multisite | Cc: |
Description
The WP_MS_Sites_List_Table class used to display sites in the admin terminal. With work on the WP_Site_Query progressing, we should move away from using sql query to display sites.
Attachments (5)
Change History (31)
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
9 years ago
#5
@
9 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 4.6
Yes, it should. :)
Once get_sites()
is in, a patch here should make more sense.
#6
@
9 years ago
Specifically, we have an opportunity to improve the search functionality at the same time. See #33185 for some previous work by the great @johnbillion.
#7
@
9 years ago
- Summary changed from WP_MS_Sites_List_Table should us WP_Site_Query to WP_MS_Sites_List_Table should use WP_Site_Query
#8
@
9 years ago
- Keywords has-patch added; needs-patch removed
36675.diff makes use of WP_Site_Query
in WP_MS_Sites_List_Table
. Replacing the SQL with the arguments for the site query was pretty straightforward. In the implementation I use WP_Site_Query
directly to be able to access the $found_sites
property as well (and get_sites()
isn't merged yet anyway). Since the list table expects the sites to be arrays, I only query the IDs and run them through get_site( $id, ARRAY_A )
afterwards.
A small exception was how the search in the list table currently works. WP_Site_Query
didn't natively support the way wildcards are used in the search there yet. That's why I also made a minor modification in WP_Site_Query
itself, allowing to pass asterisks into the search string. When doing so, these asterisks will be replaced by %
and not be escaped so that they act as wildcards. With that change it is possible to continue using the list table search like before.
Regarding the total count of sites, I set the argument no_found_rows
to false as long as we're not in a "large network" (similar like the count wasn't calculated before in that case).
Another thing we might wanna think about is whether we should move the search by IP address bit into WP_Site_Query
or not. While it would enhance the search, I think that's a very rare use-case and it needs to make an additional database query (or JOIN), so I personally would leave it in the WP_MS_Sites_List_Table
class - still that's something to consider.
#9
@
9 years ago
- Type changed from enhancement to task (blessed)
Patch looks great! This could easily go in as is, but the esc_like()
needs a few things:
- More eyes
- Pairity with wildcard user search
- Tests to ensure its secure
- Tests to ensure it is the same and/or better than it was before
Tests could come later, and we could revert the wildcard stuff if we are anxious to get this in early, which I am.
#11
@
9 years ago
On 36675.diff, setting of the order
value can probably be simplified since whitelisting is already handled in WP_Site_Query::parse_order()
.
Current:
<?php if ( $order_by ) { $args['order'] = ( isset( $_REQUEST['order'] ) && 'DESC' === strtoupper( $_REQUEST['order'] ) ) ? "DESC" : "ASC"; }
Proposed:
<?php if ( $order_by && isset( $_REQUEST['order'] ) ) { $args['order'] = $_REQUEST['order']; }
#12
@
9 years ago
I'd also propose that we take this opportunity to rename the $s
variable to $search
because self-documenting code ftw.
#15
@
9 years ago
36675.2.diff makes a few changes:
- Eliminate the
is_subdomain_install()
check so that domain and path are always searched for the term. - Similarly, only order by domain when
blogname
is passed. - Prepend and append
%
to the built wildcard query inget_search_sql()
so thatth*s
turns into%th%s%
rather thanth%s
.
The removal of is_subdomain_install()
is probably overreaching, especially with large subdirectory installations. That probably needs to go back. With subdomain configurations, we can arbitrarily search domain/path at will.
We talked about this briefly, but it would be nice to support a search that includes domain and path - wp.org/foo
. That's probably a future ticket, otherwise this may go on a while. :)
setting of the order value can probably be simplified since whitelisting is already handled in
WP_Site_Query::parse_order()
It can be simplified a bit, but the isset()
for order should probably still be used in the ternary so that ASC
is assigned by default rather than order not being populated at all.
Another thing we might wanna think about is whether we should move the search by IP address bit into WP_Site_Query or not.
I don't think so. It's super rare and not part of the $wpdb->blogs
table, so we can let it be it's own thing.
#16
follow-up:
↓ 19
@
9 years ago
36675.2.diff has an issue when a persistent object cache is in use: On first load all sites are correctly fetched and a pagination is visible. On second load only the first page is visible, no pagination.
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
#19
in reply to:
↑ 16
;
follow-up:
↓ 20
@
9 years ago
Replying to ocean90:
36675.2.diff has an issue when a persistent object cache is in use: On first load all sites are correctly fetched and a pagination is visible. On second load only the first page is visible, no pagination.
36675.3.diff uses the same approach as the comments list table for this. The first query is handled through get_sites()
and then a query to get the total count is handled through another get_sites()
call.
Existing tests have been updated to show new results. I did not add tests for wildcards in the middle of a search term, though that appears to be fine.
I did not add the subdomain check back, though I think that should still happen.
#20
in reply to:
↑ 19
@
9 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
- Owner set to jeremyfelt
- Status changed from reopened to accepted
Replying to jeremyfelt:
Existing tests have been updated to show new results. I did not add tests for wildcards in the middle of a search term, though that appears to be fine.
36675.4.diff includes the latest patch for #35791, which adds support for search columns in WP_Site_Query
and tests for wildcard searching.
I did not add the subdomain check back, though I think that should still happen.
This is back. If subdomain, then results are ordered by domain and both domain and path are searched. If subdirectory, results are ordered by path and only path is searched.
I believe the current patch fixes:
- #33185, which is a general ticket for how site search barely worked because only portions of the domain or path were being searched against.
- #24833, which describes search not working for a subdomain configuration in which domains have been changed arbitrarily.
- #21837, which is similar to both of the above tickets but deals specifically with domain search not working in which the primary site is itself a subdomain.
- #26580 isn't necessarily fixed, but the use of
WP_Site_Query
makes it much easier to add a filter similar tousers_list_table_query_args
.
Related: #35791