Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 8 years ago

#21459 closed task (blessed) (fixed)

Cleanup switch_to_blog() and restore_current_blog()

Reported by: ryan's profile ryan Owned by: ryan's profile ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4.1
Component: Multisite Keywords: has-patch
Focuses: Cc:

Description

These should be as fast and light as possible.

Attachments (15)

21459.diff (8.7 KB) - added by ryan 12 years ago.
21459.2.diff (11.3 KB) - added by ryan 12 years ago.
21459.3.diff (12.0 KB) - added by ryan 12 years ago.
21459.4.diff (12.7 KB) - added by ryan 12 years ago.
21459.5.diff (13.8 KB) - added by ryan 12 years ago.
Update wp_upload_dir()
21459.typo.patch (927 bytes) - added by SergeyBiryukov 12 years ago.
21459-filter.diff (1.8 KB) - added by ryan 12 years ago.
21459-no-switched.diff (2.7 KB) - added by ryan 12 years ago.
21459-no-switched.2.diff (2.9 KB) - added by ryan 12 years ago.
21459-no-switched-ut.diff (2.4 KB) - added by ryan 12 years ago.
21459.6.diff (382 bytes) - added by ryan 12 years ago.
21459.testcase.ms-switch-inteference.php (1016 bytes) - added by joostdekeijzer 12 years ago.
testcase for current_user_can_for_blog and get_blog_option interference
21459.testcase.ms-switch-speed.php (2.6 KB) - added by joostdekeijzer 12 years ago.
speed testcase for current_user_can_for_blog
21459.7.diff (1.4 KB) - added by joostdekeijzer 12 years ago.
21459-is_main_site-ut.diff (873 bytes) - added by ryan 12 years ago.

Download all attachments as: .zip

Change History (64)

@ryan
12 years ago

@ryan
12 years ago

@ryan
12 years ago

@ryan
12 years ago

#1 follow-up: @scribu
12 years ago

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

Version 0, edited 12 years ago by scribu (next)

#2 @wonderboymusic
12 years ago

  • Keywords has-patch added

#3 in reply to: ↑ 1 @ryan
12 years ago

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.

#4 @ryan
12 years ago

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 avoids 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.
Last edited 12 years ago by ryan (previous) (diff)

#5 @ryan
12 years ago

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.
Last edited 12 years ago by ryan (previous) (diff)

@ryan
12 years ago

Update wp_upload_dir()

#6 @ancawonka
12 years ago

  • Cc anca-wp@… added

#7 @wpmuguru
12 years ago

21459.5.diff looks good enough to start testing with.

#10 @SergeyBiryukov
12 years ago

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

#11 @ryan
12 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [21523]:

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

#12 @ryan
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Might do ms_is_switched() to wrap the switched bool.

#13 @ryan
12 years ago

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

#14 @ryan
12 years ago

In [21560]:

Introduce is_ms_switched(). see #21459

#15 follow-up: @ryan
12 years ago

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.

#16 in reply to: ↑ 15 @nacin
12 years ago

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?

#17 @nacin
12 years ago

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".

#18 @ryan
12 years ago

In [21561]:

s/is_ms_switched/ms_is_switched/ see #21459

@ryan
12 years ago

#19 follow-up: @ryan
12 years ago

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

#21 in reply to: ↑ 19 @ryan
12 years ago

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.

#22 @ryan
12 years ago

In [21581]:

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

#23 @nacin
12 years ago

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

#24 @ryan
12 years ago

In [21595]:

Restore blog_option_* filter. see #21459

#25 @BenjaminRMueller
12 years ago

  • Cc BenjaminRMueller added

#26 @joostdekeijzer
12 years ago

  • Cc joost@… added

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

#27 @ryan
12 years ago

Is that running before the init action fires?

#28 @ryan
12 years ago

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

@ryan
12 years ago

#29 @ryan
12 years ago

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]

Last edited 12 years ago by ryan (previous) (diff)

@joostdekeijzer
12 years ago

testcase for current_user_can_for_blog and get_blog_option interference

@joostdekeijzer
12 years ago

speed testcase for current_user_can_for_blog

#30 @joostdekeijzer
12 years ago

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

#31 @ryan
12 years ago

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.

#32 @nacin
12 years ago

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

#33 @nacin
12 years ago

Uh. This looks wrong, in restore_current_blog() —

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

#34 follow-up: @ryan
12 years ago

That's the same as ever though.

#35 in reply to: ↑ 34 @nacin
12 years ago

  • Type changed from enhancement to task (blessed)

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.

#36 @ryan
12 years ago

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

#37 @ryan
12 years ago

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

#38 @joostdekeijzer
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

$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.

#39 @ryan
12 years ago

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

#40 @ryan
12 years ago

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

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

#41 @ryan
12 years ago

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

#43 @ryan
12 years ago

#19235 seems more likely.

#44 @nacin
12 years ago

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.

#45 follow-up: @ryan
12 years ago

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.

#46 @ryan
12 years ago

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

#47 in reply to: ↑ 45 @nacin
12 years ago

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.

#48 @ryan
12 years ago

Opened #22090.

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


8 years ago

Note: See TracTickets for help on using tickets.