Cleanup switch_to_blog() and restore_current_blog()

[ryan]

These should be as fast and light as possible.

[scribu]

If WP_Roles is always initialized now, we should remove the check from add_role(), get_role() etc.

[ryan]

Replying to scribu:

If WP_Roles is always initialized now, we should remove the check from add_role(), get_role() etc.

I'd like to get rid of the checks but want to avoid fatals for plugins performing user ops before init runs.

[ryan]

Summary of changes in 21459.4.diff

wp-includes/admin-bar.php:

• Replace get_admin_url() and get_home_url() with admin_url() and home_url() and place them inside a switch/restore. Likewise replace current_user_can_for_blog() with current_user_can(). This avoids doing multiple switch restores.

wp-includes/ms-blogs.php:

• Deprecate the $validate argument to switch_to_blog(). This avoid a not very necessary call to get_blog_details() possibly saving a few queries.
• Use $_wp_switched and $_wp_switched_stack instead of $switched and $switched_stack to make it less likely these globals will be stomped.
• Use GLOBALS to access blog_id and other globals. I've preferred this style lately since it makes it obvious a global is being used and avoids global blog_id being stomped by a local variable.
• Lose some is_object() checks. wp_get_current_user() always returns an object, for example.
• Call the new WP_Roles::reinit() method.

wp-includes/class-wp-xmlrpc-server.php:

• Replace current_user_can_for_blog() with current_user_can() and move it inside the switch/restore pair. This eliminates a switch/restore.

wp-includes/capabilities.php:

• Use array_keys() instead of $role => $data since $data is unused. I *think* this is a bit faster.
• Introduce WP_Roles::reinit(). This reinitializes WP_Roles and is used after switch_to_blog() has already update the blog ID in the wpdb object. If a global roles array is being used instead of the db, reinit is skipped.
• current_user_can_for_blog() now does a switch/restore. It didn't before meaning it could be reinitializing the user with the wrong role information for the current blog.

wp-includes/ms-settings.php:

• Define $_wp_switched_stack and $_wp_switched. This way switch_to_blog() and restore_current_blog() can rely on it being set.

wp-settings.php:

• Instantiate the WP_Roles global. This was it is always defined during init. To remove the WP_Roles checks from WP_Role and WP_User this would probably have to move before plugins are loaded, which might not be a good thing.

[ryan]

Todo:

• Update wp_upload_dir() to use the new switched global or maybe a new function for accessing the switched state.
• Get rid of switched since we can derive whether we are switched by checking if the switch stack is empty or not.

[ryan]

Update wp_upload_dir()

[wpmuguru]

21459.5.diff looks good enough to start testing with.

[ryan]

[21485]

[ryan]

​[UT958]

[SergeyBiryukov]

21459.typo.patch​ fixes a typo in [21485] (and replaces a tab with a space in line 450).

[ryan]

In [21523]:

Fix phpdoc for switch_to_blog() and restore_current_blog(). Props SergeyBiryukov. fixes #21459

[ryan]

Might do ms_is_switched() to wrap the switched bool.

[ryan]

And switch_to_blog() no longer returns false. Maybe we should drop the @return since it is meaningless now.

[ryan]

In [21560]:

Introduce is_ms_switched(). see #21459

[ryan]

Perhaps the *_option() functions should check is_ms_switched() and not run the option filters if switched. I've seen several places that set filters assuming the current blog will always be current. When filtering things like home this can lead to interesting hijinks.

[nacin]

Replying to ryan:

Perhaps the *_option() functions should check is_ms_switched() and not run the option filters if switched. I've seen several places that set filters assuming the current blog will always be current. When filtering things like home this can lead to interesting hijinks.

I've seen the same. Perhaps we can drop-in the _blog_option filters for these cases?

[nacin]

Thinking we drop $_wp_switched and have is_ms_switched() just check $wp_switched_stack.

I'm still waffling on whether to bring back $switched for compatibility with plugins checking for it.

Also thinking ms_is_switched() rather than is_ms_switched(). I think it works better as a prefix, rather than "is the multisite switched".

[ryan]

In [21561]:

s/is_ms_switched/ms_is_switched/ see #21459

[ryan]

With 21459-no-switched.2.diff, the multisite unit tests exhibit some odd behavior. Tests_Post_Objects::test_get_post_array() fatal errors inside get_post() because WP_Post::filter() is returning false. The subsequent to_array() call is thus done on a non-object. Looking at $this in ::filter(), the post is js filtered. This is not expected. I suspect a cache problem.

Fatal error: Call to a member function to_array() on a non-object in /.../unit-tests/trunk/wordpress/wp-includes/post.php on line 406

[ryan]

​[UT989]

[ryan]

Replying to ryan:

With 21459-no-switched.2.diff, the multisite unit tests exhibit some odd behavior. Tests_Post_Objects::test_get_post_array() fatal errors inside get_post() because WP_Post::filter() is returning false. The subsequent to_array() call is thus done on a non-object. Looking at $this in ::filter(), the post is js filtered. This is not expected. I suspect a cache problem.

This is because I forgot to replace one instance of _wp_switched with ms_is_switched() in Tests_MS::test_switch_restore_blog(). The assertion failed resulting in the rest of the test not being run. When test_get_post_array() ran the cache was switched to the wrong blog. I think we need to rollback cache changes between tests.

[ryan]

In [21581]:

Resurrect the switched global. Some are still using it. see #21459

[nacin]

I am thinking that $switched should be unused in core. Set, but never referenced. Comment every set with a "hey, use ms_is_switched()". ms_is_switched() can return ! empty( stack ).

[ryan]

In [21595]:

Restore blog_option_* filter. see #21459

[joostdekeijzer]

Since [21485] current_user_can_for_blog and get_blog_option interfere resulting in unexpected results.

The changes to current_user_can_for_blog seem to be the cause...

For example, in below code $sort_array will contain only the name of the 'current' blog (the blog the user initially navigated to):

// code taken from my network-shared-media plugin (could probably be optimized for speed...)
foreach ( (array) $blogs as $details ) {
	if ( !current_user_can_for_blog( $details['blog_id'], 'upload_files') ) {
		continue;
	}

	$details['name'] = get_blog_option( $details['blog_id'], 'blogname' );
	$this->blogs[] = $details;
	$sort_array[] = strtolower ( $details['name'] );
}

I will try to write a testcase later this week

[ryan]

Is that running before the init action fires?

[ryan]

Or we need to directly use the $current_user global rather than using wp_get_current_user().

[ryan]

That doesn't seem to be it since the object seems to be passed around by ref as it should be.

I added some assertions to make sure the cap_key changes during switch. ​[UT1009]

[joostdekeijzer]

testcase for current_user_can_for_blog and get_blog_option interference

[joostdekeijzer]

speed testcase for current_user_can_for_blog

[joostdekeijzer]

The example code is running after init in my plugin.

I've written a test-case but I must say I can't replicate the issue there. It is my first test-case ever so it's possible I'm doing something wrong...

Looking into this I was wondering why the current_user_can_for_blog function was changed. The "original" didn't use the switch_to_blog() function anywhere but $WP_User->for_blog( $blog_id ) which is very fast. Written a speed-test-case for this :-)

[ryan]

It was changed because switch_to_blog() re-inits the roles. This wasn't done before so for_blog() wasn't doing a full switch.

[nacin]

Anything left here? Should we deprecate current_user_can_for_blog() the way we'd like to deprecate get_blog_option()? (#21432)

[nacin]

Uh. This looks wrong, in restore_current_blog() —

	if ( ! $GLOBALS['switched'] )
		return false;

[ryan]

That's the same as ever though.

[nacin]

Replying to ryan:

That's the same as ever though.

Yeah, I guess this should be either $GLOBALS['_wp_switched_stack'] or ms_is_switched(). We kept $switched around for compatibility, but should avoid using it in case plugins clash with it.

[ryan]

In [22015]:

Don't read from the switched global. Instead use _wp_switched_stack. switched is retained for back compat and should not be directly read since it is prone to stompage by plugins. see #21459

[joostdekeijzer]

$wp_roles->reinit() calls get_option which uses $wp_object_cache, so it should be run after the cache object has been switched to the new blog_id. See attached patch.

This actually also fixes the problem described in comment:26.

I haven't been able to create a testcase to "prove" the issue, but I can attach a simple plugin which shows the problem, if needed.

[ryan]

Nice find! Now that I see it, I can't believe I missed it. Thanks.

[ryan]

In [22087]:

Reinit roles and the current user after switching the cache to the new blog in switch_to_blog() and restore_current_blog(). Props joostdekeijzer. fixes #21459

[ryan]

Ignore the unrelated comment.php check in. Reverted in [22088].

[ocean90]

A bug? ​http://wordpress.org/support/topic/switch_to_blog-does-not-properly-set-wp_upload_dir-vars

[ryan]

#19235 seems more likely.

[nacin]

It's the result of #19235, but worth reporting it here too.

Before #19235, switching never worked because it relied on constants. Now, it calls is_main_site(). Unfortunately, is_main_site() uses $current_blog->blog_id rather than $blog_id or $wpdb->blogid, and therefore does not respond when the site is switched.

That means that when switched, is_main_site() returns a different value than is_main_site( get_current_blog_id() ).

That's enough to make me want to start switching $current_blog and $current_site, see #14190. Unfortunately, I have no idea what it'd break.

Switching to the cryptic, seemingly nonsensical, and unnecessary-but-necessary is_main_site( get_current_blog_id() ) should fix this.

[ryan]

Any reason not to use get_current_blog_id() in is_main_site()?

And going further, eliminate use of global $current_blog and instead use get_blog_details( get_current_blog_id() ) or perhaps a new get_current_blog() function.

[ryan]

Tests for is_main_site(). That last assertion fails due to use of $current_blog.

[nacin]

Replying to ryan:

Any reason not to use get_current_blog_id() in is_main_site()?

And going further, eliminate use of global $current_blog and instead use get_blog_details( get_current_blog_id() ) or perhaps a new get_current_blog() function.

The only real purpose of $current_blog is it can be set in sunrise.php and then most of ms-settings.php gets skipped.

I can't think of a reason to not use get_current_blog_id() in is_main_site(). I highly doubt someone is relying on it to be ignorant of switches.

I'd like to eliminate $current_blog usage, definitely. We could make it so get_blog_details() with no arguments returns the current blog's details, forgoing a new function.

[ryan]

Opened #22090.