WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#32450 closed task (blessed) (fixed)

WP_Site class

Reported by: jeremyfelt Owned by: jeremyfelt
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch
Focuses: multisite Cc:

Description

Historically, the global $current_blog has been a stdClass and has not had any methods attached to it. We should better define this as an instance of a WP_Site object.

This ticket should:

  • Introduce the WP_Site class and have base properties and methods for interacting with a site object.
  • Apply the new class where it can be used within core.
  • Provide tests for any methods introduced.

See #31985 for the WP_Network class equivalent of this ticket.

Attachments (4)

32450.01.patch (11.3 KB) - added by johnjamesjacoby 3 years ago.
32450-switch_to_blog_wp_site_support.diff (707 bytes) - added by BinaryKitten 3 years ago.
add support to switch_to_blog for WP_Site
32450.diff (6.8 KB) - added by jeremyfelt 2 years ago.
32450.2.diff (6.2 KB) - added by jeremyfelt 2 years ago.

Download all attachments as: .zip

Change History (43)

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


3 years ago

#2 @johnjamesjacoby
3 years ago

32450.01.patch does the following:

  • Introduces a (largely incomplete) base WP_Site class using WP_Post for inspiration
  • Moves the guts of get_site_by_path() into a static get() method
  • Includes a get_instance() method similar to WP_Post::get_instance(), but I don't see a practical use case for it yet

This patch isn't very great, but is... something. We should iterate towards replacing get_site_by_path() usages with new WP_Site( $args ) usages instead.

Last edited 3 years ago by johnjamesjacoby (previous) (diff)

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 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

#6 follow-up: @obenland
3 years ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

@jeremyfelt, do you think this will still make 4.3 with two weeks left until beta?

#7 in reply to: ↑ 6 @jeremyfelt
3 years ago

Replying to obenland:

@jeremyfelt, do you think this will still make 4.3 with two weeks left until beta?

@obenland - likely not, though I'm going to keep the 4.3 milestone attached to it for a bit longer just in case. :) Slack log from yesterday.

#8 @jeremyfelt
3 years ago

  • Keywords dev-feedback added
  • Milestone changed from 4.3 to Future Release

I'd like continued progress on this ticket, #31985, #32504, etc... for the remainder of the 4.3 cycle so that we can get things in early in 4.4 for more complete testing.

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


3 years ago

#10 @BinaryKitten
3 years ago

@jeremyfelt, I'd like to suggest the following additions:

  • adding support to pass WP_Site to switch_to_blog
  • adding a method "switch_to" to the WP_Site that would pass itself/id to the switch_to_blog

this would allow for things like

switch_to_blog( get_site_by_path( 'mysitepath' ) );

or

$my_site = get_site_by_path( 'mysitepath' );
$my_site->switch_to();

both of which would end up performing the same code in the end, but allows for promoting the WP_Site to a primary object.

@BinaryKitten
3 years ago

add support to switch_to_blog for WP_Site

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 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

#14 @jeremyfelt
3 years ago

  • Milestone changed from Future Release to 4.4
  • Type changed from enhancement to task (blessed)

32450.01.patch is a good start. I don't think it's far from an initial commit that models the database in the same way that WP_Network, WP_Comment, and WP_Post all do.

We should aim to introduce that first, and apply it to areas of core as possible. It may be a little tougher than WP_Network only because multisite does actually handle multiple sites by default. :)

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


3 years ago

This ticket was mentioned in Slack in #core by drew. View the logs.


3 years ago

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

#19 @jeremyfelt
3 years ago

  • Milestone changed from 4.4 to Future Release

This hasn't gotten the attention it deserves after the initial patch. We should try to make some progress on this before the 4.5 cycle.

@jeremyfelt
2 years ago

#20 follow-up: @jeremyfelt
2 years ago

  • Milestone changed from Future Release to 4.5

32450.diff is my latest version of this that I forgot to upload a while ago. :)

Moving this to 4.5

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


2 years ago

#22 in reply to: ↑ 20 @johnjamesjacoby
2 years ago

Replying to jeremyfelt:

32450.diff is my latest version of this that I forgot to upload a while ago. :)

Moving this to 4.5

This seems doable today IMO. And the diff between our two patches is the start of WP_Site_Query.

Do we want to consolidate the individual status properties immediately afterwards? It would be nice to avoid having all of those separate object variables if we plan to collapse them in a future release.

#23 follow-up: @johnjamesjacoby
2 years ago

The blog-details & blog-lookup cache keys contain the same data, which is essentially the details of a blog look-up (imagine that.) I'm not confident how to best combine these into 1 canonical place, since sites are queried both by site ID and by domain & path, but I think it'd be nice to have a single bucket for sites rather than 2.

As I type this, I wonder if blog-lookup is meant to piggy-back domain aliasing. Maybe worth giving it a peek, too.

#24 @johnjamesjacoby
2 years ago

There is also the is_blog_installed cache key that we might be able to tack onto this. A site should know if it's installed correctly, especially if that's a thing whole WordPress installations need to be cognizant of.

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


2 years ago

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


2 years ago

@jeremyfelt
2 years ago

#27 in reply to: ↑ 23 @jeremyfelt
2 years ago

  • Keywords has-patch added; needs-patch dev-feedback removed
  • Status changed from assigned to accepted

Another refresh in 32450.2.diff.

In the previous patch, I was hoping we could populate the blogname, home, siteurl, and post_count properties as part of the WP_Site object. This relies on get_option() which in turn relies on wp-includes/formatting.php—which is not included during the bootstrap process—and on setting the $wpdb table prefixes. Chicken meet egg. :)

That's been backed out to focus only on mirroring the database structure for the time being. This seems to provide a pretty clear path.

The latest patch also adds sites to our global groups and ensures the site's sites cache is cleared in clean_blog_cache(). (See #29247, #35251)


Replying to johnjamesjacoby:

Do we want to consolidate the individual status properties immediately afterwards? It would be nice to avoid having all of those separate object variables if we plan to collapse them in a future release.

Definitely open to this. Am I imagining it correctly as $wp_site->status with $wp_site->spam still accessible (via _get())? Are there any downsides to consolidating those?

Replying to johnjamesjacoby:

The blog-details & blog-lookup cache keys contain the same data, which is essentially the details of a blog look-up (imagine that.) I'm not confident how to best combine these into 1 canonical place, since sites are queried both by site ID and by domain & path, but I think it'd be nice to have a single bucket for sites rather than 2.

It would be nice to pre-populate these on object creation if possible. We might be able to rework get_blog_details() a bit, especially as WP_Site_Query starts to be more obvious.

Replying to johnjamesjacoby:

There is also the is_blog_installed cache key that we might be able to tack onto this. A site should know if it's installed correctly, especially if that's a thing whole WordPress installations need to be cognizant of.

This one looks a little harder since it deals with the options tables. Worth exploring in another ticket once we have WP_Site in?


@toscho and I also had a discussion yesterday in Slack around public/private properties. After poking around some more, I'm still inclined to leave them as public.

The $current_blog object has been around and stompable for years. We've also provided a blog_details filter since pre-merge that allows a site's object to be directly filtered. It's tempting to protect the properties, but I'd rather error on the side of back-compat here.

I can see adding getters/setters to help ensure the quality of the data.

#28 @jeremyfelt
2 years ago

In 36393:

Multisite: Introduce the WP_Site class.

  • A WP_Site object initially matches a row from wp_blogs.
  • A site can be retrieved by its ID through WP_Site::get_instance().
  • Adds sites to the global cache group and captures instance lookups.
  • The multisite bootstrap now ensures $current_blog is an instance of WP_Site.

Props johnjamesjacoby, jeremyfelt.
See #32450.

#29 @ocean90
2 years ago

In 36413:

Multisite: Add the global cache group sites to restore_current_blog() and wp_start_object_cache().

See #32450.

#30 @DrewAPicture
2 years ago

In 36495:

Docs: Make some minor improvements to inline docs for WP_Site, introduced in [36393].

  • Uses third-person singular verbs in method summaries
  • Adds an @static tag to the WP_Site::get_instance() DocBlock
  • Adjusts return types for WP_Site::get_instance() to the more explicit WP_Site|false

See #32450. See #32246.

This ticket was mentioned in Slack in #core by drew. View the logs.


2 years ago

#32 @mikeschroder
2 years ago

Is there anything left here?

#33 @jeremyfelt
2 years ago

Probably not - I'm going to take one more closer look tomorrow to make sure, but then I'll close it up.

#34 @jeremyfelt
2 years ago

In 36894:

Docs: Update param/return types for WP_Site in ms-blogs.php

  • get_blog_details() now returns a WP_Site object.
  • clean_blog_cache() is now called with a WP_Site object.

See #32450.

#35 @jeremyfelt
2 years ago

In 36895:

Docs: Update the return type for get_active_blog_for_user()

This is now a WP_Site object.

See #32450.

#36 @jeremyfelt
2 years ago

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

Closing this out, thanks everyone! We can open new tickets for bugs if they come up.

#37 follow-up: @spacedmonkey
2 years ago

Should wp_get_sites return a array of WP_Site objects?

#38 in reply to: ↑ 37 @johnjamesjacoby
2 years ago

Replying to spacedmonkey:

Should wp_get_sites return a array of WP_Site objects?

Definitely. And we'll need to do some instance juggling (like WP_Post does) to make this happen on a broader scale.

#39 @spacedmonkey
2 years ago

It should be simple enough. Something like this should work.

$sites = $wpdb->get_results( $query );
$site_results = array();
foreach( $sites as $site ){
	$site_results[] = new WP_Site( $site );
}
Note: See TracTickets for help on using tickets.