WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 7 weeks ago

#40228 assigned enhancement

Use get_sites in get_blog_details

Reported by: spacedmonkey Owned by: flixos90
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch has-unit-tests dev-feedback
Focuses: multisite Cc:

Description

Currently get_blog_details performs raw sql queries to get data out of the blogs tables. This data is then cached. However how the caching is done in the function is horrible. It generates three different caches that store the whole wp_site object in cache.

wp_cache_delete( $blog_id , 'blog-details' );
wp_cache_delete( $blog_id . 'short' , 'blog-details' );
wp_cache_delete(  $domain_path_key, 'blog-lookup' );    

This function should be refactor to use get_sites and relay on the caching built into wp_site_query

Attachments (2)

40228-tests.diff (1.6 KB) - added by flixos90 3 months ago.
40228.diff (11.8 KB) - added by flixos90 3 months ago.

Download all attachments as: .zip

Change History (19)

#1 @spacedmonkey
3 months ago

Here is what a get_blog_details function would look like using get_sites. It basically creates different args and passes it to get_sites and returns 1 site.


function get_blog_details( $fields = null, $get_all = true ) {
        $args = array();
        if ( is_array( $fields ) ) {
                if ( isset($fields['blog_id']) ) {
                        $args['site__in'][] = $fields['blog_id'];
                } 
                if ( isset($fields['domain']) ){
                        $domains = array( $fields['domain'] );
                        if ( substr( $fields['domain'], 0, 4 ) == 'www.' ) {
                                $nowww = substr( $fields['domain'], 4 );
                                $domains[] = $nowww;
                        }
                        $args['domain__in'] = $domains;
                }
                if( isset($fields['path']) ) {
                        $args['path'] = $fields['path'];
                }

        } elseif ( is_numeric( $fields ) ){
                $args['site__in'][] = $fields;
        } elseif ( $fields && is_string( $fields ) ) {
                $current_network = get_network();
                $slug = trim( $fields, '/' );

                if ( is_subdomain_install() ) {
                        $domain = $slug . '.' . preg_replace( '|^www\.|', '', $current_network->domain );
                        $path = $current_network->path;
                } else {
                        $domain = $current_network->domain;
                        $path = $current_network->path . $slug . '/';
                }
                $args['path'] = $path;
                $args['domain'] = $domain;
        } else {
                $args['site__in'][] = get_current_blog_id();
        } 
        
        if( empty( $args ) ){
                return false;
        }
        
        $args['number'] = 1;
        $sites = get_sites( $args );
        
        if( empty( $sites ) ){
                return false;
        }
        
        $details = array_shift( $sites );
        $blog_id = $details->blog_id; 

        if ( $get_all ) {
                switch_to_blog( $blog_id );
                $details->blogname   = get_option( 'blogname' );
                $details->siteurl    = get_option( 'siteurl' );
                $details->post_count = get_option( 'post_count' );
                $details->home       = get_option( 'home' );
                restore_current_blog();
        
        /**
         * Filters a blog's details.
         *
         * @since MU
         * @deprecated 4.7.0 Use site_details
         *
         * @param object $details The blog details.
         */
        $details = apply_filters_deprecated( 'blog_details', array( $details ), '4.7.0', 'site_details' );
        }
        return $details;
}

#2 @flixos90
3 months ago

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

Thanks for the code snippet @spacedmonkey!

I think we should rather direct our efforts to get_site_by() (#40180) and then wrap that function with get_blog_details(). See https://core.trac.wordpress.org/ticket/40064#comment:19

I'll probably work on a patch later that takes your above proposal into account.

#3 @spacedmonkey
3 months ago

I like the idea of get_site_by however it is not a full replacement for all the params that get_blog_details. get_term_by only supports getting a term by one field, such as slug. However get_blog_details allows an array to passed and lookup is done by multiple fields. This array maps pretty well to the args of get_sites.

Looking at this code again, I might skip get_sitrs if a number is passed and just use get_site.

#4 @flixos90
3 months ago

I think we should also support an array in get_site_by(). We can definitely wrap it in get_blog_details(). I have written a patch for that, which I'll upload in a bit.

#5 @spacedmonkey
3 months ago

Here is a much less tidy version of the function but does things a much more quicker way. If you pass nothing or a number, just use get_site. This means that we don't need to spin up a while wp_site_query, which will use a lot lesss memory.


function get_blog_details( $fields = null, $get_all = true ) {
        $args = array();
        $details = null;
        if ( is_numeric( $fields ) ){
                $details = get_site( $fields );
        } elseif ( is_array( $fields ) ) {
                if ( isset($fields['blog_id']) ) {
                        $args['site__in'][] = $fields['blog_id'];
                } 
                if ( isset($fields['domain']) ){
                        $domains = array( $fields['domain'] );
                        if ( substr( $fields['domain'], 0, 4 ) == 'www.' ) {
                                $nowww = substr( $fields['domain'], 4 );
                                $domains[] = $nowww;
                        }
                        $args['domain__in'] = $domains;
                }
                if( isset($fields['path']) ) {
                        $args['path'] = $fields['path'];
                }

        } elseif ( $fields && is_string( $fields ) ) {
                $current_network = get_network();
                $slug = trim( $fields, '/' );

                if ( is_subdomain_install() ) {
                        $domain = $slug . '.' . preg_replace( '|^www\.|', '', $current_network->domain );
                        $path = $current_network->path;
                } else {
                        $domain = $current_network->domain;
                        $path = $current_network->path . $slug . '/';
                }
                $args['path'] = $path;
                $args['domain'] = $domain;
        } else {
                $details = get_site();
        } 
        if( null === $details ){
              if( empty( $args ) ){
                    return false;
               }
        
               $args['number'] = 1;
               $sites = get_sites( $args );
        
               if( empty( $sites ) ){
                     return false;
               }
        
               $details = array_shift( $sites );
        }
        $blog_id = $details->blog_id; 

        if ( $get_all ) {
                switch_to_blog( $blog_id );
                $details->blogname   = get_option( 'blogname' );
                $details->siteurl    = get_option( 'siteurl' );
                $details->post_count = get_option( 'post_count' );
                $details->home       = get_option( 'home' );
                restore_current_blog();
        
        /**
         * Filters a blog's details.
         *
         * @since MU
         * @deprecated 4.7.0 Use site_details
         *
         * @param object $details The blog details.
         */
        $details = apply_filters_deprecated( 'blog_details', array( $details ), '4.7.0', 'site_details' );
        }
        return $details;
}

#6 @flixos90
3 months ago

40228-tests.diff are two additional unit tests that verify the behavior described by @stephdau in https://core.trac.wordpress.org/ticket/40180#comment:3 works as expected.

#7 @flixos90
3 months ago

In 40317:

Multisite: Add further unit tests for get_blog_details().

These tests verify that the returned site object is iterable and contains the expected properties.

See #40228, #40180.

@flixos90
3 months ago

#8 @flixos90
3 months ago

  • Keywords has-patch has-unit-tests dev-feedback added
  • Milestone changed from Awaiting Review to 4.8
  • Version 3.0 deleted

40228.diff shows a possible implementation of get_blog_details() using get_site_by() (see #40180). Since the patch relies on that function, it is required that you apply the patch on #40180 as well in order to test.

All regular get_blog_details() tests pass after the change, however I had to adjust / partly remove some tests related to the blog-details and blog-lookup caches, since these now don't exist any longer. There are two tests failing related to the site-details cache, however that is due to a bug in setting that cache, for which I'll open a ticket shortly.

The code in get_blog_details() has obviously been drastically reduced, and by using get_site_by() the patch fulfills the original requirement of using the more efficient get_sites() / get_site() functions. Admittedly there is one really weird line in the new code ($site->detail = $site->detail;), but it actually does what it is supposed to do (PHP magic method weirdness) - however this code is one of the reasons that I think we should recommend using get_site_by() over get_blog_details() going forward (see more below).

I think this approach makes more sense than refactoring get_blog_details() itself. get_site_by() is more explicit in its usage and does not explicitly set the "site details" on the object (which is a bad practice since they're now lazy-loaded, but we have to keep it in get_blog_details() for BC. We could use get_site_by() in other core functions, like get_id_from_blogname() for example. These are some of the reasons why I think get_site_by() is viable to have in core although it serves a rather similar use-case like get_blog_details(). Let's not deprecate get_blog_details(), but let's provide a better alternative for new development going forward.

Last edited 3 months ago by flixos90 (previous) (diff)

#9 @flixos90
3 months ago

Regarding the above bug, I opened #40247 and added a patch. So in order to have the unit tests pass, the patches from #40247, #40180 and this very ticket need to be applied.

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


3 months ago

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


3 months ago

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


3 months ago

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


3 months ago

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


2 months ago

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


2 months ago

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


7 weeks ago

#17 @flixos90
7 weeks ago

  • Milestone changed from 4.8 to Future Release

Let's get back to this soon, once we figured out what to do about #40180 (introduce a new function and use it vs adjusting get_blog_details() directly).

Note: See TracTickets for help on using tickets.