Deprecate `get_blog_details()`

[flixos90]

As discussed earlier and stated in ​https://make.wordpress.org/core/2016/11/04/multisite-focused-changes-in-4-7/, let's deprecate the get_blog_details() function. Its blog_details filter has already been deprecated in 4.7, and this is the next step on the roadmap.

Maybe we can also remove the blog_details filter occurrence in the WP_Site class that was temporarily introduced in 4.7 (I'm not sure anymore that this was a clever idea, but we should be safe to remove it from that location since it was already introduced as being deprecated).

[lukecavanagh]

@flixos90

Seems like that function is just defined here.

https://core.trac.wordpress.org/browser/tags/4.7/src/wp-includes/ms-blogs.php#L122

[spacedmonkey]

Personally I think we should keep the function around. It was a corner stone of multisite for a long time and might be widely used in third party code.

However, looking at the code in this function is a mess. It is so confusing and is poorly documented. The internals should be refactored to use wp_site_query and unnecessary caches removed.

[flixos90]

@spacedmonkey I don't think it makes sense to adjust get_blog_details() to use WP_Site_Query. The get_site() function is a clear replacement for get_blog_details() with major benefits, so the latter should be deprecated in favor of get_site() for all the same reasons you mentioned as well. We already announced its deprecation in the 4.7 multisite dev note, so people should be prepared. Furthermore, the function will probably always stay around, it will only be deprecated. :)

Adding to @lukecavanagh's comment that we already removed all get_blog_details() occurrences as part of #37102 and #38349, so we're safe to deprecate the function.

[spacedmonkey]

Either way, deprecated or not we will have to support the internals of this function. This function is a mess with a nest of if else statements. It also generates and number of different caches, which are not used else where in core. These caches will have to be invalidated forever more if the code in the function isn't refactored.

Consider the following sql call that appears in the function.

        $blog = $wpdb->get_row( $wpdb->prepare( "SELECT * FROM $wpdb->blogs WHERE domain IN (%s,%s) AND path = %s ORDER BY CHAR_LENGTH(domain) DESC", $nowww, $fields['domain'], $fields['path'] ) );

This query on the blogs table should be removed with the caches it generates and replaced with wp_site_query. Wp_site_query has caching built in and is much better maintained, making this function easier to support going forward.

[spacedmonkey]

Here is what a get_blog_details function would look like using get_sites.

<?php

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

This makes for much clearer code.

[flixos90]

@spacedmonkey I still don't think it makes sense to do this. Since we deprecate this function, we don't need to improve it anymore. More importantly, improvements can always introduce backward-compatibility issues, and in such a case I don't think it's worth the hassle.
I think the guideline here should be to make sure deprecated functions still work, but they do not necessarily work as well as their recommended counterpart. The terrible code of that function plus the introduction of get_site() which does exactly the same, only in a better way, are what I consider the reasons for deprecating.

[spacedmonkey]

There are a number of reason to refactor this function. Firstly, the code is extremely hard to read. Readability is important in maintain of code and this is really hard. The second and more important is how this function caching is horrible. This function generates 3 caches. Keys are listed below.

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

All of these caches store the whole wp_site object in the object store. This is wasteful and resource intensive. These caches should store the object id and not the object itself. if we don't refactor, this bad caching will remain and possibly make problems down the line. It also means that in the clean_blog_cache function will be have to keep the invalidation (cache deletes) forever. So will have to do these deletes even through those cache will more than likely never be hit. Deletin empty caches is also wasteful.

[stephdau]

One of the things that get_blog_details() allows for, that get_site() doesn't, is the ability to get the details with a domain and a path. What would be the plan for that?

At the moment, I'm leaning towards thinking that @spacedmonkey has the right idea.

[stephdau]

Replying to flixos90:

Adding to @lukecavanagh's comment that we already removed all get_blog_details() occurrences as part of #37102 and #38349, so we're safe to deprecate the function.

I don't think that makes it safe to deprecate. It's great to have replaced Core's use of its own function, but Core isn't the only one using it (by design). You have to consider what the impact on 3rd-party integrations will be. And I don't think that filling people's logs with deprecation notices is a viable one, especially if we don't provide a direct equivalent for them to use (see my previous comment).

[flixos90]

@stephdau It's a good point that get_site() isn't a full replacement for get_blog_details(). Regarding deprecation, I still think we should deprecate that function because of its far-from-optimal approach. But I agree, there should be a more straightforward alternative. You can very easily replace get_blog_details() with a get_sites() query and array_shift() in case you're querying by domain or path, but maybe it makes more sense to introduce an additional function get_site_by() that basically wraps these. get_site_by() could then become a direct replacement which has all the optimizations that we put into get_site() and get_sites().

[flixos90]

I opened #40180 for a get_site_by(), so let's continue related discussion there.

[stephdau]

👍

[spacedmonkey]

@stephdau get_site_by will likely not help you. If get_site_by is like get_term_by you will not be to get a site by domain and path. get_term_by only lets you get a term by one field, such as slug.

There are already other helper function that might help. get_site_by_path is used in the bootstrap process but could also be used to look up a site by domain and path. get_site_by_path Is a new function, is well supported with tests and is not going anywhere.

[spacedmonkey]

Related: #40228

[jeremyfelt]

I think we should hold off on deprecating get_blog_details(). :)

We eliminated its use within core, which solves most of the issue we had with it. We deprecated the blog_details filter, which encourages anyone customizing a site's details to look to the new location instead.

As there is no one to one replacement (it's unique that way), I don't think there's much harm in letting it be. As @stephdau mentions, the person hours involved in making this change are pretty high because it's used frequently. And because it's not a one to one replacement, the chance for error when people make that change is higher.

If we do not deprecate it, then we should also update the internals as best we can so that it can take the most advantage of newer cache keys, etc...

Separately, it's worth repeating... get_site() is a one to one replacement if only passed a site's ID. get_site( 234 ) is a better option than get_blog_details( 234 ).

And I'm not sure if this is the best place to start this conversation, but what if get_site() supported a partial site object in addition to an ID? It could then initiate a get_sites() query based on what information is available. That would bring it much closer to a replacement, and then easier to argue for deprecation of get_blog_details().

[spacedmonkey]

Should we continue the refactor talk on this other ticket then #40228

[flixos90]

@jeremyfelt That makes sense. I think we can make get_site_by() a one-to-one replacement though if we develop it in a proper way. The only exception would be that it would always return a WP_Site instead of plain object. Then get_blog_details() could call get_site_by() and transform its return value into a plain object, including taking care of whether to $get_all or not.
Maybe we can deprecate the function then, maybe not. I think we should at least discourage its usage then.

[flixos90]

As I have now looked at the issue more closely, I think that implementing get_site_by() as a one-to-one replacement for get_blog_details() doesn't make sense. Also due to the aforementioned reasons, I agree that we shouldn't deprecate the function, but rather simply recommend get_site_by() going forward.

Let's continue discussion of the enhancements in #40180 and #40228, and there are also some patches to review and test. :)

[stephdau]

<3 :)