WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#31148 closed enhancement (invalid)

Extend wp_get_sites() to support queries on domain and path

Reported by: earnjam Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: needs-testing has-patch 4.6-early needs-unit-tests close
Focuses: multisite Cc:

Description

wp_get_sites() was introduced in [25445]. At that time @nacin noted:

I think the roadmap for this function, by the way, should be to replace all direct queries to $wpdb->blogs in WP_MS_Sites_List_Table, and ideally any other non-complex direct queries to $wpdb->blogs elsewhere.

In order to do this, it needs to be extended to be able to support queries on both domain and path as well as across all networks.

This could also be the first step toward replacing domain_exists() in #30233.

Attachments (4)

31148.diff (5.6 KB) - added by earnjam 3 years ago.
Add support for domain, path and bypassing the wp_is_large_network check
31148.2.patch (5.6 KB) - added by spacedmonkey 2 years ago.
31148.3.patch (5.7 KB) - added by spacedmonkey 2 years ago.
31148.4.patch (6.1 KB) - added by spacedmonkey 2 years ago.

Download all attachments as: .zip

Change History (20)

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


3 years ago

#2 @earnjam
3 years ago

  • Summary changed from Extend wp_get_sites() to support queries on domain, path and across all networks to Extend wp_get_sites() to support queries on domain and path

@earnjam
3 years ago

Add support for domain, path and bypassing the wp_is_large_network check

#3 @earnjam
3 years ago

  • Keywords dev-feedback needs-testing has-patch added

31148.diff is a first stab at adding parameters for domain, path and a boolean to allow the function to run against large networks.

If it's going to be used for some of these direct queries on the wp_blogs table, or replace domain_exists(), then it needs to have the ability to search large networks when required. This patch keeps the original functionality of returning an empty array on a large network by default unless you explicitly pass it a true value.

One thing I did notice is that I think wp_is_large_network() is only checking the size of the current network, so If you run this function from within a network with > 10,000 sites, but are trying to search against a different small network, it would return an empty array. I guess the boolean param I added would allow you to bypass that, but something to think about.

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


3 years ago

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


3 years ago

#6 @jeremyfelt
3 years ago

  • Milestone changed from Awaiting Review to Future Release

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.


2 years ago

#9 @spacedmonkey
2 years ago

Had another bash at this ticket. Like the idea but added some useful options to it.

I think this should be a phase one of using the wp_get_sites function as a way of calling the blogs table throughout WordPress. See #35791 for a list of functions where the logic should remove if this patch is merged. Later we can replace the logic in the wp_get_site function with WP_Site_Query class.

Related: #35791 #35697 #30233

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


2 years ago

#11 follow-up: @jeremyfelt
2 years ago

  • Keywords 4.6-early needs-unit-tests added; dev-feedback removed

Thanks @spacedmonkey, looking good! (And thanks @earnjam for the initial stab at this.)

Some notes while going through the patch:

  • In the first iteration, I think I'd prefer sticking to ids and all for fields. We can work through #35697 a bit more to decide what else would be useful.
  • We previously restricted to wp_is_large_network(). We should probably consider moving away from that, especially with the default limit at 100.
  • Supporting date queries would be interesting for the registered and last_updated fields.
  • We could probably support path in orderby as well. Someone may want to order by one of the other properties, but it seems unlikely.
  • When ids is passed for fields, we should use $wpdb->get_col() instead (see WP_Query) so that we have an array of IDs rather than the same associative array.

I think things are in a nice place. Some unit tests would be helpful as well. It would also be interesting to start iterating on this now into a WP_Site_Query for 4.6 rather than waiting.

#12 in reply to: ↑ 11 @spacedmonkey
2 years ago

Replying to jeremyfelt:

I think things are in a nice place. Some unit tests would be helpful as well. It would also be interesting to start iterating on this now into a WP_Site_Query for 4.6 rather than waiting.

Not exactly sure what you mean here? Do you mean continue on this patch or drop it and continue on with WP_Site_Query work?

Are people happy with the new params? network_id, site_id, path and domain all allow arrays passed to them. Should we add *__in params to bring it into line with other functions like get_posts?

I have updated patch with feedback. I have kept domain, as I think there is a valid use for returning that. :D

#13 @spacedmonkey
2 years ago

Added a per hook pre_wp_get_sites. This can be used to hot wire wp_get_sites to load results from object cache.

#14 @flixos90
2 years ago

A lot of progress going on here :)

Should we work changes from #35697 into this patch here as well? The issues are really closely related, so maybe it's better if we close the other one as duplicate.

We would need to think of which values should be possible for $args['fields']. I think allowing some associative combinations would be helpful, like id=>domain or id=>path (see patch in #35697) since these are combinations that are probably needed often (depending on whether the setup uses domains or paths).

#15 @thomaswm
2 years ago

  • Keywords close added

wp_get_sites() will be deprecated. See #36994. Its replacement, get_sites(), introduced in r37616, already supports queries on domain and path.

#16 @jeremyfelt
2 years ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

Agreed, let's close this. Thanks everyone for the work on this ticket. It helped get things moving on #35791, which introduced WP_Site_Query and get_sites().

Note: See TracTickets for help on using tickets.