[wp-trac] [WordPress Trac] #35791: WP_Site_Query class

[WordPress Trac]

#35791: WP_Site_Query class
-------------------------------------------------+-------------------------
 Reporter:  spacedmonkey                         |       Owner:
     Type:  task (blessed)                       |      Status:  new
 Priority:  normal                               |   Milestone:  4.6
Component:  Networks and Sites                   |     Version:  4.4
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch needs-testing needs-unit-  |     Focuses:  multisite
  tests                                          |
-------------------------------------------------+-------------------------
Changes (by jeremyfelt):

 * keywords:  has-patch needs-testing => has-patch needs-testing needs-unit-
     tests


Comment:

 Walking through this a bit tonight to get familiar. First of all, great
 stuff! Thank you for all the work and discussion so for. :100:

 Replying to [comment:5 spacedmonkey]:
 > At some point this patch should be broken up into two. One for
 wp_site_query and another for the implementation of it in core functions.

 Yes, please. :) It would be beneficial to have a patch that introduces
 `WP_Site_Query` and then different patches for how it is applied
 throughout core. Right now, unit tests go a bit haywire and it would be
 easier to determine the cause if I could add/remove portions. When it
 comes time to commit, this will also be the approach.

 > - orderby CHAR_LENGTH on domain and path. Need a solution for this
 > - Bootstrap loader, am I loading it to late / soon.

 The queries in `ms-load.php` / `get_site_by_path()` are a special beast.
 It's probably best to leave them be completely, or save that for a later
 time. This function should really only be useful during bootstrap anyway.
 Removing these changes helps most of the unit test errors.

 > - Should update_site_cache do more, warm other caches? Should it have a
 hook?

 Hmm. There are 8 different keys that we clear in `clean_blog_cache()`. It
 looks like we could prime the short `blog-details` cache without requiring
 any more data. The rest is probably fine. This is probably something we
 can probably add to `WP_Site::get_instance()` at some point too. I think I
 remember @johnjamesjacoby mentioning something along those lines at some
 point.

 > - Comments need a lot of work.

 The are a **lot** of docs. :) Pinging @DrewAPicture for a docs review at
 some point, though additional work from others is very welcome. :)

 > - This query object should be used to create a sites endpoint in the api

 Absolutely. It's probably beneficial to start a PR here -
 https://github.com/WP-API/wp-api-site-endpoints - once we get things
 locked in.

 > Not sure that this is secure / best way of doing this
 > {{{#!php
 > implode( "', '", $wpdb->_escape( $this->query_vars['path__not_in'] ) ) .
 "' )
 > }}}

 At first glance, I think we're okay. I'll dig into that more soon.

 Replying to [comment:13 flixos90]:
 > Replying to [comment:12 spacedmonkey]:
 > > I think it should be an option to select what format the function
 returns.
 >
 > Sounds good, we just need to make sure that ARRAY_A is the default.

 +1 `ARRAY_A` should be default. We should approach adding different return
 formats for `wp_get_sites()` in a different ticket.

 > > As for count, I don't agree with your change. This class is based on
 wp_comment_query, which does have count. I think this shouldn't change.
 >
 > I personally prefer to have it as a possible value for fields since one
 parameter to modify the type of output should be sufficient. For example,
 using both the fields and count parameters in one query wouldn't make
 sense.

 I lean toward the `count` parameter on this rather than part of fields,
 though it's worth thinking about.

 Other notes:
 * Code style: Let the code around different changes be in most cases. It's
 sometimes painful to see broken code standards, but with such a large
 change like this, it is helpful for the patch/commit history to be as
 clean as possible. An example here is `wp-includes/http.php`, where proper
 brackets have been added to code that is not changing.
 * Unit tests: As mentioned above, some of the unit tests are failing now.
 We need to make sure those stay passing **and** we should introduce a set
 of tests specifically for `WP_Site_Query`.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/35791#comment:15>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform