Site icon functions using `ms_is_switched()` incorrectly

[jdgrimes]

get_custom_logo(), get_site_icon_url(), and has_custom_log() all use ms_is_switched() in the same way:

        // At the top of the function:
        if ( is_multisite() && (int) $blog_id !== get_current_blog_id() ) {
                switch_to_blog( $blog_id );
        }

        // ...

        // At the bottom of the function:
        if ( is_multisite() && ms_is_switched() ) {
                restore_current_blog();
        }

However, what if our code looks something like this:

switch_to_blog( $blog_id );

// Do some stuff...

// Grab the icon for this blog.
$icon_url = get_site_icon_url( 512, 'full', $blog_id );

// Let's do some more stuff...but wait!
// Uh-oh, now we aren't switched anymore!
if ( $blog_id !== get_current_blog_id() ) {
     echo 'Oops.'
}

You see, if we are already switched to the site when one of these functions is called, ms_is_switched() will already be true. Thus, the function will call restore_current_blog() internally, even though it didn't call switch_to_blog() internally. So calling these functions unexpectedly pops one site off from the switched stack.

[achbed]

Potential patch for this issue. Does not include unit tests.

[flixos90]

Good catch @jdgrimes! Moved it to the 4.7 milestone.

Thanks for the patch @achbed. For next time, it would be great if you could also comment on the ticket (if only to mention that you uploaded a patch) because the upload process itself does not ping people. So it was actually good that you mentioned it on Slack, but by commenting you can do it from the ticket. :)

I will work on unit tests today and upload them a bit later.

[flixos90]

38253.diff​ adds unit tests to ensure the switched state is preserved for the three functions.

I made one additional change: In the initial clause of each function I also check whether $blog_id is not empty as otherwise we don't need to call switch_to_blog() at all. While the process is not expensive in that case, I think not calling the function makes sense in this case.

[achbed]

I didn't realize that the comment area of the patch upload didn't send notifications. Thanks for the info.

Also, good idea on reducing extra function calls when they're not needed.

[jeremyfelt]

Replying to flixos90:

I made one additional change: In the initial clause of each function I also check whether $blog_id is not empty as otherwise we don't need to call switch_to_blog() at all. While the process is not expensive in that case, I think not calling the function makes sense in this case.

I like this change. Calling switch_to_blog( 0 ); doesn't seem like something we should be in the habit of doing. :)

[jeremyfelt]

In 38786:

Multisite: Maintain switched state in site icon/logo functions.

Adjusts get_custom_logo(), get_site_icon_url(), and has_custom_logo() so that when called in a switched state, the original switched stack is not adjusted.

Props achbed, flixos90.
Fixes #38253.