Make WordPress Core

Opened 10 months ago

Last modified 10 months ago

#59173 new enhancement

Improve optimizations of get_site_url by not using switch_to_blog

Reported by: pers's profile PerS Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch needs-testing
Focuses: multisite, performance Cc:

Description

I belive a better approach would be to use the WP_Site_Query query, as in:

<?php
function get_site_url( $blog_id = null, $path = '', $scheme = null ) {
        if ( empty( $blog_id ) || ! is_multisite() ) {
                $url = get_option( 'siteurl' );
        } else {

                // Use WP_Site_Query to get the site URL.
                $site = get_site( $blog_id );

                $url = $site->domain . $site->path;

                // If the network has a custom domain, use that instead.
                if ( $site->domain !== \get_network()->domain ) {
                        $url = get_network()->domain . $site->path;
                }
        }

        $url = set_url_scheme( $url, $scheme );

        if ( $path && is_string( $path ) ) {
                $url .= '/' . ltrim( $path, '/' );
        }

        /**
         * Filters the site URL.
         *
         * @since 2.7.0
         *
         * @param string      $url     The complete site URL including scheme and path.
         * @param string      $path    Path relative to the site URL. Blank string if no path is specified.
         * @param string|null $scheme  Scheme to give the site URL context. Accepts 'http', 'https', 'login',
         *                             'login_post', 'admin', 'relative' or null.
         * @param int|null    $blog_id Site ID, or null for the current site.
         */
        return apply_filters( 'site_url', $url, $path, $scheme, $blog_id );
}


Change History (6)

#1 @rajinsharwar
10 months ago

  • Type changed from defect (bug) to enhancement

This ticket was mentioned in PR #5054 on WordPress/wordpress-develop by @PerS.


10 months ago
#2

  • Keywords has-patch added

Improve optimisations of get_site_url() by not using switch_to_blog, instead use the WP_Site_Query query

#3 @johnbillion
10 months ago

  • Keywords needs-testing added

Thanks for the ticket and PR @PerS. Some questions that will need answering before this change can be made:

  • Does get_site_url() have sufficient test coverage to make this change safe?
  • This change means the filters in get_option() will no longer fire when fetching the site URL, is that a concern? For example domain mapping plugins might break.
  • What sort of performance improvement does this result in?

#4 @PerS
10 months ago

Hi John,

The existing unit tests should be sufficience, and I've added a domain mapping check.

Performance wise, my main reason for this patch is to avoid switch_to_blog, even with the latest fixes it's a slow function. And for a super admin it's triggered a lot (ref query monitor).

Last edited 10 months ago by PerS (previous) (diff)

#5 @manfcarlo
10 months ago

I strongly recommend not to accept this patch. The reasons should be obvious. It replaces a query of one field in one table to a totally different field in a totally different table. The effects would be significant and wide-reaching, and to describe it as a "performance" tune-up is misleading.

I'm no fan of switch_to_blog and like to see it avoided, but you can't do it by throwing the baby out with the bathwater.

#6 @jeremyfelt
10 months ago

I don't think wp_blogs.domain + wp_blogs.path is a one to one replacement for the siteurl option. It's more aligned with the home option instead.

I don't have a specific example where this would break, but I'd be very wary of changing the existing option call.

One particular case that would need testing that is probably not covered in our existing tests is WordPress installed in a subdirectory and configured for multisite.

Note: See TracTickets for help on using tickets.