WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#36935 closed enhancement (fixed)

Implement lazy-loading blog details in `WP_Site`

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

Description

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.

Attachments (10)

36935.diff (5.9 KB) - added by flixos90 2 years ago.
36935.2.diff (6.4 KB) - added by flixos90 2 years ago.
36935.3.diff (11.7 KB) - added by flixos90 2 years ago.
36935.4.patch (10.7 KB) - added by spacedmonkey 2 years ago.
36935.4.diff (10.7 KB) - added by spacedmonkey 2 years ago.
36935.5.diff (11.8 KB) - added by flixos90 2 years ago.
36935.6.diff (12.2 KB) - added by jeremyfelt 23 months ago.
36935.7.diff (5.1 KB) - added by jeremyfelt 23 months ago.
36935.8.diff (5.4 KB) - added by jeremyfelt 23 months ago.
36935.9.diff (5.9 KB) - added by flixos90 23 months ago.

Download all attachments as: .zip

Change History (41)

@flixos90
2 years ago

#1 @flixos90
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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

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


2 years ago

#3 @jeremyfelt
2 years ago

  • Focuses multisite added
  • Milestone changed from Awaiting Review to 4.6

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

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


2 years ago

#5 @chriscct7
2 years ago

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

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


2 years ago

@flixos90
2 years ago

#7 @flixos90
2 years ago

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

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


2 years ago

@flixos90
2 years ago

#9 @flixos90
2 years ago

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()
Last edited 2 years ago by flixos90 (previous) (diff)

#10 @flixos90
2 years ago

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().

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


2 years ago

#12 @spacedmonkey
2 years ago

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.

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


2 years ago

#14 @spacedmonkey
2 years ago

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 );
         }

@spacedmonkey
2 years ago

@flixos90
2 years ago

#15 @flixos90
2 years ago

  • Keywords needs-testing removed

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().

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


23 months ago

#17 @jeremyfelt
23 months ago

  • Owner changed from flixos90 to jeremyfelt
  • Status changed from assigned to reviewing

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
23 months ago

#18 @jeremyfelt
23 months ago

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
23 months ago

#19 follow-up: @jeremyfelt
23 months ago

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?
Last edited 23 months ago by jeremyfelt (previous) (diff)

#20 in reply to: ↑ 19 ; follow-up: @flixos90
23 months ago

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?

#21 in reply to: ↑ 20 @jeremyfelt
23 months ago

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
23 months ago

#22 follow-up: @jeremyfelt
23 months ago

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?

#23 in reply to: ↑ 22 @flixos90
23 months ago

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
23 months ago

#24 @flixos90
23 months ago

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.

#25 @jeremyfelt
23 months ago

  • Keywords commit needs-dev-note added

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. :)

#26 @flixos90
23 months ago

See #37235 for the new action.

#27 @jeremyfelt
23 months ago

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

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.

#28 follow-up: @spacedmonkey
23 months ago

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

#29 in reply to: ↑ 28 @jeremyfelt
23 months ago

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' );
...

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


23 months ago

#31 @ocean90
23 months ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.