WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#36675 closed task (blessed) (fixed)

WP_MS_Sites_List_Table should use WP_Site_Query

Reported by: spacedmonkey Owned by: jeremyfelt
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)

36675.diff (5.5 KB) - added by flixos90 3 years ago.
36675.2.diff (5.2 KB) - added by jeremyfelt 3 years ago.
36675.3.diff (6.8 KB) - added by jeremyfelt 3 years ago.
36675.4.diff (11.4 KB) - added by jeremyfelt 3 years ago.
36675.5.diff (6.6 KB) - added by jeremyfelt 3 years ago.

Download all attachments as: .zip

Change History (31)

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


3 years ago

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


3 years ago

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


3 years ago

#5 @jeremyfelt
3 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 @johnjamesjacoby
3 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 @DrewAPicture
3 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

@flixos90
3 years ago

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

#10 @flixos90
3 years ago

  • Keywords needs-unit-tests added

#11 @DrewAPicture
3 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'];
}
Last edited 3 years ago by DrewAPicture (previous) (diff)

#12 @DrewAPicture
3 years ago

I'd also propose that we take this opportunity to rename the $s variable to $search because self-documenting code ftw.

#13 @ajv9540
3 years ago

  • Resolution set to worksforme
  • Status changed from new to closed

#14 @ocean90
3 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

@jeremyfelt
3 years ago

#15 @jeremyfelt
3 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 in get_search_sql() so that th*s turns into %th%s% rather than th%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: @ocean90
3 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.

#17 @jeremyfelt
3 years ago

In 37633:

Multisite: Add initial tests for WP_MS_Sites_List_Table

Different tests are used for subdomain and subdirectory installs as domain and path are searched differently for each. Only trailing wildcard searches are tested because leading wildcards are not yet supported.

See #36675.

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


3 years ago

@jeremyfelt
3 years ago

#19 in reply to: ↑ 16 ; follow-up: @jeremyfelt
3 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.

@jeremyfelt
3 years ago

#20 in reply to: ↑ 19 @jeremyfelt
3 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 to users_list_table_query_args.

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


3 years ago

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


3 years ago

@jeremyfelt
3 years ago

#24 @jeremyfelt
3 years ago

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

In 37736:

Multisite: Use WP_Site_Query to power WP_MS_Sites_List_Table.

WP_Site_Query provides for a cleaner prepare_items() method. It significantly improves the search experience in the sites list table:

  • In a subdomain configuration, domain and path are searched for a provided terms.
  • In a subdirectory configuration, path is searched for a provided term.
  • The full domain is searched in a subdomain configuration rather than the portion not matching the network's domain.
  • Terms are searched as %term% by default. Adding * in the middle of a term will search %te%rm%.

Props flixos90, Fab1en.
Fixes #33185, #24833, #21837, #36675.

#25 @DrewAPicture
3 years ago

  • Keywords needs-docs added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Nice work! Let's add a changelog entry to the DocBlock for prepare_items() to note that it was converted to use WP_Site_Query.

#26 @DrewAPicture
3 years ago

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

In 37739:

Docs: Add a missing summary and @since version to the DocBlock for WP_MS_Sites_List_Table::prepare_items().

Also adds a changelog entry for 4.6.0 noting the changeover to get_sites().

Fixes #36675. See #21837, #24833 and #33185.

Note: See TracTickets for help on using tickets.