#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)
Change History (64)
#3
in reply to:
↑ 1
@
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
@
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.
#5
@
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.
#10
@
12 years ago
21459.typo.patch fixes a typo in [21485] (and replaces a tab with a space in line 450).
#11
@
12 years ago
- Owner set to ryan
- Resolution set to fixed
- Status changed from new to closed
In [21523]:
#12
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Might do ms_is_switched() to wrap the switched bool.
#13
@
12 years ago
And switch_to_blog() no longer returns false. Maybe we should drop the @return since it is meaningless now.
#15
follow-up:
↓ 16
@
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
@
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
@
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".
#19
follow-up:
↓ 21
@
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
@
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.
#23
@
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 ).
#26
@
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
#28
@
12 years ago
Or we need to directly use the $current_user global rather than using wp_get_current_user().
#29
@
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]
#30
@
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
@
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
@
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
@
12 years ago
Uh. This looks wrong, in restore_current_blog() —
if ( ! $GLOBALS['switched'] ) return false;
#35
in reply to:
↑ 34
@
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.
#38
@
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.
#44
@
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:
↓ 47
@
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.
#47
in reply to:
↑ 45
@
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.
If WP_Roles is always initialized now, we should remove all the check from add_role(), get_role() etc.