Implement lazy-loading blog details in `WP_Site`

[flixos90]

Let's move the functionality from get_blog_details() that sets the additional properties into the WP_Site class. However this should not happen when instantiating a site object, but only when one of these additional parameters is requested (magic getters).

This will bring a minor performance improvement, but more importantly it ensures that these values are available at all times. At the current state, it is hard to tell whether a site object has or hasn't these properties available.

An important point to consider for the implementation is that we need to keep the filter blog_details in place.

[flixos90]

36935.diff​ is an initial patch. I added a magic getter (and issetter) to WP_Site for the additional properties blogname, siteurl, post_count and home which were originally dealt with in get_blog_details().

Whenever one of these properties is now requested through a WP_Site object, a new private method get_details() is called which basically contains the code from get_blog_details(). To be backwards compatible with the filter blog_details (that is now part of this method as well), a raw object copy of the site object is created there. Note that I got rid of the {$blog_id}short cache key since it is of no use here (and it actually contains exactly the same like the sites cache key that was introduced with WP_Site.

The function get_blog_details() now only contains the code to retrieve the site object, including taking care of the blog-lookup cache key. The $get_all parameter is not of any use at this point since all site objects now automatically lazy-load these additional properties as described above. We might wanna deprecate that parameter.

get_blog_details() now isn't of any more use than getting a site object by its domain, path or id (for the latter, get_site() is the better function though). This function should probably make use of WP_Site_Query now that it is merged in. However I didn't want to include this in this first iteration (I'm not sure whether we should do it in this ticket at all).

[jeremyfelt]

+1

We'll need to test this extensively and make sure our unit tests are comprehensive.

One note, because I've run into this before - untrailingslashit() is in wp-includes/formatting.php, which is not available until after multisite's bootstrap is complete. If get_option() is called to populate the siteurl or home properties before then, it will break.

We need to either move formatting.php or untrailingslashit() up in the chain.

[flixos90]

The original patch needed an update after 37657. Therefore 36935.2.diff​ integrates the changes from the original patch into the updated codebase. It also adds the additional properties that are available through the get_details() method to the class doc block as @property-read.

After 37657 a bug with get_blog_details() came up (see https://core.trac.wordpress.org/ticket/36717#comment:20) - this should be fixed with this patch (the previous patch already had the fix, this new patch was only to integrate the changes).

[flixos90]

The patch 36935.3.diff​ has the following updates:

• introduce site-details cache group for WP_Site::get_details()
• thus remove blog-details cache group for that functionality (it's still used with get_id_from_blogname_* and *_user_count keys
• add the new site-details cache group to global groups for cache
• add the new group to and remove the old groups from clean_blog_cache()
• really make 100% sure that get_blog_details() returns a WP_Site object and never a plain object
• fix a possible bug (related to #36717) in update_blog_details() by using the to_array() method of WP_Site instead of get_object_vars()

[flixos90]

Reminder: In today's enhancement scrub we thought about (silently) deprecating get_blog_details() after this ticket is done - and at the same time, replace all usage of the function in Core with get_site().

[spacedmonkey]

Updated the patch. Know it does better type checking on value returned from cache. Also moved the filter after the value comes from cache. It means that the values going into cache are filtered. Only filter after it comes out. This makes sense if you have a dynamic filter, that changes on context.

[spacedmonkey]

Just idea but we could do something like this as well.

<?php
function get_blog_details( $fields = null, $get_all = true ) {
        global $wpdb;
         if ( is_numeric( $fields ) && $get_all ) {
               return get_site( $fields );
         }

[flixos90]

36935.5.diff​ fixes a get_object_vars( $site ) in update_blog_details() which doesn't work since 37657. It also silently deprecates get_blog_details().

[jeremyfelt]

Leaving a note as this came up during bug scrub tonight—if we get this in before beta 1, we should avoid the deprecation of get_blog_details() and instead just implement the lazy loading of properties with the new site-details cache key. It will take us a release or so to replace all uses of get_blog_details(), so we need to be cautious with the approach.

[jeremyfelt]

36935.6.diff​ is updated to apply cleanly against trunk.

A couple of the multisite tests attempt to access the blog-details cache key directly, which fails once it's removed from get_blog_details(). We'll need to determine how we want to handle that in 4.7 when we deprecate it.

I'll attach another patch that removes the get_blog_details() changes.

[jeremyfelt]

36935.7.diff​ still introduces the get_details() method to WP_Site. This allows for lazy loading of extended site details. It does not deprecate get_blog_details().

A couple notes:

• I changed the filter from blog_details to site_details to avoid any conflict with the filter that we would have needed to maintain back-compat with. This may help make a cleaner break if we keep get_blog_details() available and suggest that everyone move to get_site().
• Details aren't available when get_site() is used in sunrise.php. Should isset() return false when an option is not available?

[flixos90]

Replying to jeremyfelt:

Details aren't available when get_site() is used in sunrise.php. Should isset() return false when an option is not available?

Sounds good to me. How exactly are we gonna determine that? An action having run before? The globals being set?

[jeremyfelt]

Replying to flixos90:

Replying to jeremyfelt:

Details aren't available when get_site() is used in sunrise.php. Should isset() return false when an option is not available?

Sounds good to me. How exactly are we gonna determine that? An action having run before? The globals being set?

On second thought, just returning false might be the right answer. We probably shouldn't validate isset as a way to check if the property is there. It will always be there, just sometimes not populated fully.

I did run into another issue.

If you attempt to access the site details before they can be populated with get_option() (in sunrise.php), then they will be cached as false and future access attempts will also receive false. get_details() needs to skip caching of a property if false is returned from get_option().

[jeremyfelt]

36935.8.diff​ loops through the lazy-loaded properties to check for false values. If any are found, then it skips the caching of results.

Does that seem reasonable?

[flixos90]

Replying to jeremyfelt:

36935.8.diff​ loops through the lazy-loaded properties to check for false values. If any are found, then it skips the caching of results.

Does that seem reasonable?

Generally I like this approach. Only I'm not sure whether it's more performant the way you do or alternatively to iterate through array( 'blogname', 'siteurl', 'post_count', 'home' ) and check whether they exist in object and are false instead. Whichever way we choose, I think we should add a break statement after setting $cache_details to false.

[flixos90]

I thought about an a little different approach in 36935.9.diff​. Considering we had several cases where certain data is not available in sunrise.php (can't recall where exactly, but it was in more tickets than just here), I think we should add a new action ms_loaded that is run at the end of ms-settings.php, after site and network have been detected and WPDB and cache have been setup.

This gives us a more simple means to check whether site details are available or not, and it actually allows us to provide an accurate result for the isset-er.

I also changed the approach for the foreach according to my above proposal. I actually think it's not even required with the new ms_loaded check, but we might still leave it in there to make sure.

Although adding a new action this close before Beta is generally not preferable, I hope we can get away with it as I think it is straightforward and we're so close before the finish line with this ticket. What do you think @jeremyfelt? If you agree with this approach, maybe we should deal with the action in a quick separate ticket for organization.

[jeremyfelt]

This does sound like a good approach, @flixos90. I like the idea of having an ms_loaded action available. It could come in useful for other sunrise related things. Like you suggested, let's do a quick ticket with that and then try to get both in.

I'm happy with the current state of this ticket, so I'm going to flag as commit. I'm mobile at the moment, but hope to be available in the next 30-60 minutes. :)

[flixos90]

See #37235 for the new action.

[jeremyfelt]

In 37918:

Multisite: Lazy load extended WP_Site properties when requested.

In the past, get_blog_details() has been used to retrieve the home, siteurl, blogname, and post_count options for a site. By lazy loading properties in a WP_Site object, we can avoid having to use get_blog_details() and instead provide the properties as needed.

This introduces the global site-details cache group in which standard objects representing the site are stored. This will one day be a replacement for the blog-details cache group that is currently used in get_blog_details().

This relies on the ms_loaded action introduced in [37916] as properties are not available via get_option() until multisite has been fully loaded.

Props flixos90.
Fixes #36935.

[spacedmonkey]

I just notice that the new cache group isnt cleared in clean_blog_cache.

[jeremyfelt]

Replying to spacedmonkey:

I just notice that the new cache group isnt cleared in clean_blog_cache.

I think it is:

function clean_blog_cache( $blog ) {
	$blog_id = $blog->blog_id;
	$domain_path_key = md5( $blog->domain . $blog->path );

	wp_cache_delete( $blog_id, 'sites' );
	wp_cache_delete( $blog_id, 'site-details' );
	wp_cache_delete( $blog_id , 'blog-details' );
...