Opened 11 years ago
Last modified 7 years ago
#25293 new defect (bug)
Switch_to_blog not switching the siteid
Reported by: | Rahe | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | minor | Version: | 3.0 |
Component: | Networks and Sites | Keywords: | has-patch dev-feedback 2nd-opinion |
Focuses: | multisite | Cc: |
Description
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.
Attachments (2)
Change History (24)
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
#8
@
8 years ago
@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.
#9
@
8 years ago
For the above concept, it could look something like this:
function switch_to_site( $site, $switch_network = false ) { if ( $switch_network && ! is_object( $site ) ) { $site = get_site( $site ); $site_id = $site->blog_id; } elseif ( is_object( $site ) ) { $site_id = $site->blog_id; $site = $site_id; } else { $site_id = $site; } $GLOBALS['_wp_switched_sites_stack'][] = $site; // 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 && ! is_object( $network ) ) { $network = get_network( $network ); $network_id = $network->id; } elseif ( is_object( $network ) ) { $network_id = $network->id; $network = $network_id; } else { $network_id = $network; } $GLOBALS['_wp_switched_networks_stack'][] = $network; // 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(); } }
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
8 years ago
#11
follow-up:
↓ 12
@
8 years ago
- Keywords has-patch dev-feedback 2nd-opinion added
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()
andrestore_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. :)
#12
in reply to:
↑ 11
;
follow-up:
↓ 13
@
8 years ago
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? :)
#13
in reply to:
↑ 12
;
follow-up:
↓ 14
@
8 years ago
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.
#14
in reply to:
↑ 13
;
follow-up:
↓ 15
@
8 years ago
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).
#15
in reply to:
↑ 14
@
8 years ago
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.
#16
@
8 years ago
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. :)
#17
follow-up:
↓ 18
@
8 years ago
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.
#18
in reply to:
↑ 17
;
follow-up:
↓ 19
@
8 years ago
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 seeWP_Site_State
andWP_Locale_State
(currentlyWP_Locale_Switcher
) to implement aWP_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?
#19
in reply to:
↑ 18
@
8 years ago
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.
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 haveswitch_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 wrapsswitch_to_blog()
and aswitch_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.