WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 19 months ago

Last modified 19 months ago

#21459 closed task (blessed) (fixed)

Cleanup switch_to_blog() and restore_current_blog()

Reported by: ryan Owned by: 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 21 months ago.
21459.2.diff (11.3 KB) - added by ryan 21 months ago.
21459.3.diff (12.0 KB) - added by ryan 21 months ago.
21459.4.diff (12.7 KB) - added by ryan 21 months ago.
21459.5.diff (13.8 KB) - added by ryan 21 months ago.
Update wp_upload_dir()
21459.typo.patch (927 bytes) - added by SergeyBiryukov 20 months ago.
21459-filter.diff (1.8 KB) - added by ryan 20 months ago.
21459-no-switched.diff (2.7 KB) - added by ryan 20 months ago.
21459-no-switched.2.diff (2.9 KB) - added by ryan 20 months ago.
21459-no-switched-ut.diff (2.4 KB) - added by ryan 20 months ago.
21459.6.diff (382 bytes) - added by ryan 20 months ago.
21459.testcase.ms-switch-inteference.php (1016 bytes) - added by joostdekeijzer 20 months 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 20 months ago.
speed testcase for current_user_can_for_blog
21459.7.diff (1.4 KB) - added by joostdekeijzer 19 months ago.
21459-is_main_site-ut.diff (873 bytes) - added by ryan 19 months ago.

Download all attachments as: .zip

Change History (63)

ryan21 months ago

ryan21 months ago

ryan21 months ago

ryan21 months ago

comment:1 follow-up: scribu21 months ago

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

Last edited 21 months ago by scribu (previous) (diff)

comment:2 wonderboymusic21 months ago

  • Keywords has-patch added

comment:3 in reply to: ↑ 1 ryan21 months 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.

comment:4 ryan21 months 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 21 months ago by ryan (previous) (diff)

comment:5 ryan21 months 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 21 months ago by ryan (previous) (diff)

ryan21 months ago

Update wp_upload_dir()

comment:6 ancawonka21 months ago

  • Cc anca-wp@… added

comment:7 wpmuguru21 months ago

21459.5.diff looks good enough to start testing with.

comment:10 SergeyBiryukov20 months ago

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

comment:11 ryan20 months 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

comment:12 ryan20 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Might do ms_is_switched() to wrap the switched bool.

comment:13 ryan20 months ago

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

comment:14 ryan20 months ago

In [21560]:

Introduce is_ms_switched(). see #21459

comment:15 follow-up: ryan20 months 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.

comment:16 in reply to: ↑ 15 nacin20 months 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?

comment:17 nacin20 months 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".

comment:18 ryan20 months ago

In [21561]:

s/is_ms_switched/ms_is_switched/ see #21459

ryan20 months ago

ryan20 months ago

ryan20 months ago

ryan20 months ago

comment:19 follow-up: ryan20 months 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

comment:21 in reply to: ↑ 19 ryan20 months 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.

comment:22 ryan20 months ago

In [21581]:

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

comment:23 nacin20 months 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 ).

comment:24 ryan20 months ago

In [21595]:

Restore blog_option_* filter. see #21459

comment:25 BenjaminRMueller20 months ago

  • Cc BenjaminRMueller added

comment:26 joostdekeijzer20 months 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

comment:27 ryan20 months ago

Is that running before the init action fires?

comment:28 ryan20 months ago

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

ryan20 months ago

comment:29 ryan20 months 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 20 months ago by ryan (previous) (diff)

joostdekeijzer20 months ago

testcase for current_user_can_for_blog and get_blog_option interference

joostdekeijzer20 months ago

speed testcase for current_user_can_for_blog

comment:30 joostdekeijzer20 months 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 :-)

comment:31 ryan20 months 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.

comment:32 nacin19 months ago

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

comment:33 nacin19 months ago

Uh. This looks wrong, in restore_current_blog() —

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

comment:34 follow-up: ryan19 months ago

That's the same as ever though.

comment:35 in reply to: ↑ 34 nacin19 months 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.

comment:36 ryan19 months 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

comment:37 ryan19 months ago

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

comment:38 joostdekeijzer19 months 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.

joostdekeijzer19 months ago

comment:39 ryan19 months ago

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

comment:40 ryan19 months 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

comment:41 ryan19 months ago

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

comment:43 ryan19 months ago

#19235 seems more likely.

comment:44 nacin19 months 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.

comment:45 follow-up: ryan19 months 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.

comment:46 ryan19 months ago

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

comment:47 in reply to: ↑ 45 nacin19 months 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.

comment:48 ryan19 months ago

Opened #22090.

Note: See TracTickets for help on using tickets.