WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

#38253 closed defect (bug) (fixed)

Site icon functions using `ms_is_switched()` incorrectly

Reported by: jdgrimes Owned by: jeremyfelt
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.3
Component: Networks and Sites Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

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.

Attachments (2)

38253-notests.patch (1.6 KB) - added by achbed 7 months ago.
Potential patch for this issue. Does not include unit tests.
38253.diff (3.7 KB) - added by flixos90 7 months ago.

Download all attachments as: .zip

Change History (10)

@achbed
7 months ago

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

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


7 months ago

#2 @flixos90
7 months ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.7
  • Owner set to flixos90
  • Status changed from new to assigned

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.

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

@flixos90
7 months ago

#3 follow-up: @flixos90
7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

#4 @achbed
7 months ago

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.

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


6 months ago

#6 @jeremyfelt
6 months ago

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

#7 in reply to: ↑ 3 @jeremyfelt
6 months ago

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

#8 @jeremyfelt
6 months ago

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

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.

Note: See TracTickets for help on using tickets.