Switch_to_blog not switching the siteid

[Rahe]

When having multiple network on multisite making the following:

switch_to_blog(1);

$options = get_site_option( 'my_option' );

restore_current_blog();

The options retrieved are the options of the current siteid and not the siteid of the switched blog.
One of the options is to make something like this :

global $wpdb;
// Get the previous siteid
$previous_site_id = $wpdb->siteid;
$previous_blog_id = $wpdb->blogid;
		
// Go to site 1
switch_to_blog(1);
		
// Set the blog siteid to 1
$wpdb->set_blog_id( 1, 1 );

// Get the options
$options = get_site_option( 'my_option' );
		
restore_current_blog();
$wpdb->set_blog_id( $previous_blog_id , $previous_site_id );

Or

// Get the previous siteid
$site_id = $wpdb->siteid;
		
// Set the blog siteid to 1
$wpdb->set_blog_id( $wpdb->blogid, 1 );
	
// Get the options
$options = get_site_option( 'my_options' );
	
$wpdb->set_blog_id( $wpdb->blogid , $site_id );

The thing is that the switch_to_blog function does not specify the switched siteid on the method $wpdb->set_blog_id if the network is not the same as the current blog.

[jeremyfelt]

One of the great things about switch_to_blog() is that it requires no database operation to succeed. It is passed a $blog_id (which should be named $site_id), which it sets to the $_GLOBALS['blog_id'], it performs $wpdb->set_blog_id() to get the correct table prefixes setup for when queries are made, and it loads in the user's capabilities for the new $blog_id.

I don't think we want to introduce a database call to find the site's network ID from the $blog_id that was passed. It would be interesting to have switch_to_blog() accept another parameter for $network_id so that the caller could explicitly determine what site and network should be available. The deprecated parameter that is there now was $validate until [13125] it looks like.

More refreshing would be a new switch_to_site( $site_id, $network_id ) function that wraps switch_to_blog() and a switch_to_network( $network_id ) function to try and limit some of the confusion at the code level. Even then, it may be that passing $network_id is a decision left to the developer as the one who knows whether a network level lookup will be needed.

[GregLone]

In 10 days, we'll celebrate this ticket 2y anniversary >_>

[Rahe]

With the new 4.6 out, is this ticket still relevant ? Or is the bug still here ?

[flixos90]

@Rahe:
The ticket is still relevant.
Actually I'm just seeing this ticket right now. It would be great to get this implemented somehow as it would also give a ticket like #37414 more value.

@jeremyfelt:
For switch_to_network() (and the related restore function), we could, for the most part, take the function from the WP Multi Network plugin. What I like about that implementation is that the entire network object is stored in the switched stack global.

For the site switching, I really like your idea of introducing a switch_to_site() for it. Maybe the second parameter could be a bool $switch_network instead of the network ID so that the function detects the network to switch to automatically. I don't think there's a use-case for switching the site and network simultaneously in a way that they don't belong together. switch_to_network() could have a similar second parameter $switch_site.

To prevent database queries on every switch, maybe both functions should accept _either_ an ID or an object. The site object has the network ID already in place (so does the network object have the site ID), so no additional query necessary. If the user passes an ID, we call get_site( $id ) (or get_network( $id ) respectively) which is not too unlikely to get a cache hit under these circumstances. In the related restore_current_*() functions, we could prevent queries in a similar way, by storing the entire objects in the switched stack globals. Maybe we should make what we store there dependent on whether the user provided $switch_network = true. If switching the network too, we store the object, if only the site, we store the ID. Then in the restore_current_*() function we can detect whether that last switch only switched the site or also the network. This way we wouldn't have DB queries there either, and we wouldn't even need a conditional parameter whether to switch the network as well since that would be automatically detected.

If we hold on to switch_to_blog() to use it internally (in switch_to_site()), we would need to adjust that to also accept an object and store that in stack. Or we create switch_to_site() from scratch with a global _wp_switched_sites_stack to not mess with switch_to_blog(), considering it deprecated at some point.

[flixos90]

For the above concept, it could look something like this:

function switch_to_site( $site, $switch_network = false ) {
	if ( $switch_network ) {
		if ( ! is_object( $site ) {
			$site = get_site( $site );
		}
		$site_id = $site->blog_id;
		$GLOBALS['_wp_switched_sites_stack'][] = $GLOBALS['current_blog'];
	} else {
		if ( is_object( $site ) {
			$site_id = $site->blog_id;
			$site = $site_id;
		} else {
			$site_id = $site;
		}
		$GLOBALS['_wp_switched_sites_stack'][] = $GLOBALS['current_blog']->blog_id;
	}

	// do rest of the regular logic with $site_id
	
	if ( $switch_network ) {
		switch_to_network( $site->site_id );
	}
}

function restore_current_site() {
	if ( empty( $GLOBALS['_wp_switched_sites_stack'] ) ) {
		return false;
	}

	$restore_network = false;

	$site = array_pop( $GLOBALS['_wp_switched_sites_stack'] );
	if ( is_object( $site ) ) {
		$site_id = $site->blog_id;
		$restore_network = true;
	} else {
		$site_id = $site;
	}

	// do rest of the regular logic with $site_id
	
	if ( $restore_network ) {
		restore_current_network();
	}
}

function switch_to_network( $network, $switch_site = false ) {
	if ( $switch_site ) {
		if ( ! is_object( $network ) {
			$network = get_network( $network );
		}
		$network_id = $network->id;
		$GLOBALS['_wp_switched_networks_stack'][] = $GLOBALS['current_site'];
	} else {
		if ( is_object( $network ) {
			$network_id = $network->id;
			$network = $network_id;
		} else {
			$network_id = $network;
		}
		$GLOBALS['_wp_switched_networks_stack'][] = $GLOBALS['current_site']->id;
	}

	// do rest of the regular logic with $network_id
	
	if ( $switch_site ) {
		switch_to_site( $network->blog_id );
	}
}

function restore_current_network() {
	if ( empty( $GLOBALS['_wp_switched_networks_stack'] ) ) {
		return false;
	}

	$restore_site = false;

	$network = array_pop( $GLOBALS['_wp_switched_networks_stack'] );
	if ( is_object( $network ) ) {
		$network_id = $network->id;
		$restore_site = true;
	} else {
		$network_id = $network;
	}

	// do rest of the regular logic with $network_id
	
	if ( $restore_site ) {
		restore_current_site();
	}
}

[flixos90]

Based on this ticket and also #37958 (plus today's Multisite chat discussion), I implemented a first take on a WP_State class in 25293.diff​. This class manages switching between sites and contains some enhanced functionality:

• not only $blog_id is switched, but also the $current_blog global
• if the network ID of the new site is different than the network ID of the old site, the network ($current_site) is switched as well (also the $wpdb->siteid property)
• it is possible to set a named breakpoint at any time (WP_State::set_breakpoint()): this will store the current state and allow to restore it later (WP_State::restore_breakpoint()) (see #37958 for more information)
• a lot of duplicate code from switch_to_blog() and restore_current_blog() has been removed by outsourcing similar parts into private methods
• the _wp_switched_stack global is gone - since it's marked private, I simply removed it and now use a class property instead; if we need to keep it for BC, that shouldn't be a problem though

The WP_State instance is currently stored in a global in this patch, but obviously I don't wanna introduce one. If we get to put this idea into Core, we should make the instance available through something like WP::get( 'state' ). Also, the name WP_State sounds pretty "global" for something only available on Multisite. Maybe another name would be more suitable.

This was basically me playing around what an improved switching structure could look like. So feel free to discuss it, take it apart, address possible performance issues. :)

[tfrommen]

Replying to flixos90:

Based on this ticket and also #37958 (plus today's Multisite chat discussion), I implemented a first take on a WP_State class in 25293.diff​. This class manages switching between sites and contains some enhanced functionality...

Personally, I don't like the patch because it introduces a class that does too many (related, and yet clearly distinct) things.

The class that allows to create a snapshot of the current network state really only needs a single method to restore. Nothing more.

If switching sites will be done by means of a (new) class, then the network state class just grabs it and uses it (instead of calling switch_to_blog()).

And if we want to account for breakpoints (which should not be handled by the network state class), these should actually be an array of NetworkState objects! Makes sense, right? :)

[flixos90]

Replying to tfrommen:

Personally, I don't like the patch because it introduces a class that does too many (related, and yet clearly distinct) things.

I'm not exactly sure why the breakpoint functionality shouldn't be handled by this class, could you clarify a bit? However if this is a problem, we can leave the two methods out first and then think about the patch as possible solution for this very ticket.

[tfrommen]

Replying to flixos90:

Replying to tfrommen:

Personally, I don't like the patch because it introduces a class that does too many (related, and yet clearly distinct) things.

I'm not exactly sure why the breakpoint functionality shouldn't be handled by this class, could you clarify a bit? However if this is a problem, we can leave the two methods out first and then think about the patch as possible solution for this very ticket.

What you described as a breakpoint is exactly what is handled by an instance of a NetworkState like I proposed, no? So breakpoints are a meta layer for network states - which is why breakpoints shouldn't be handled by the network state class. The latter just has to provide/be an immutable snapshot with the ability to apply it (i.e., restore).

[flixos90]

Replying to tfrommen:

What you described as a breakpoint is exactly what is handled by an instance of a NetworkState like I proposed, no?

Yes, indeed the above approach is an alternative to your NetworkState class.

So breakpoints are a meta layer for network states - which is why breakpoints shouldn't be handled by the network state class. The latter just has to provide/be an immutable snapshot with the ability to apply it (i.e., restore).

I would rather see the WP_State as a global controller of the switched state. A breakpoint in my opinion is indeed a snapshot of the current state, and that snapshot can be restored by the controller.

Anyway, I think we should, for the sake of this ticket, break out the set_breakpoint() and restore_breakpoint() methods plus the related $breakpoints property from the above patch and then continue discussion on the approach in #37958.

[flixos90]

25293.2.diff​ is the same as the initial patch, but without the breakpoint functionality since this belongs into #37958. Was a little too eager with the original patch. :)

[swissspidy]

Looking at this ticket, #37958 and #26511, WP_State is not a good name for this site-related class. I'd rather see WP_Site_State and WP_Locale_State (currently WP_Locale_Switcher) to implement a WP_State interface or an abstract class of such name. There's really some common ground. If we settle on consistent naming and public-facing functionality, we can also abstract later.

[flixos90]

Replying to swissspidy:

Looking at this ticket, #37958 and #26511, WP_State is not a good name for this site-related class. I'd rather see WP_Site_State and WP_Locale_State (currently WP_Locale_Switcher) to implement a WP_State interface or an abstract class of such name.

+1, the name should be more specific. Anyone has an idea better than WP_Site_State? I feel that is not accurate enough since it also indirectly handles the network state (by switching the network automatically when the site is switched). Regarding an interface, I'm not sure how this class would have anything in common with WP_Locale_State other than it switches something - might be sufficient for an interface later, or can you come up with something in particular?

[jeremyfelt]

Replying to flixos90:

Replying to swissspidy:

Looking at this ticket, #37958 and #26511, WP_State is not a good name for this site-related class.

+1, the name should be more specific. Anyone has an idea better than WP_Site_State?

Agreed. We should try to name this in a way that supports some of the things we may be able to do in the future. It's maintaining the context of a site / network in a (global) WordPress instance. The old meaning of the "site" term (MU style) would kind of work here as it's a site containing one or more networks with one or more blogs. We need to find the right terms to describe how the context of that is switched.

Regarding an interface, I'm not sure how this class would have anything in common with WP_Locale_State other than it switches something - might be sufficient for an interface later, or can you come up with something in particular?

It would be nice to identify these common areas. That each could (1) maintain a stack of contexts, (2) move to a new context, (3) restore to a previous context, and (4) restore to a specific context should mean that there are areas that overlap pretty well.

[swissspidy]

Opened a new ticket to discuss naming convention: #38098.

[spacedmonkey]

Related: #40513