Improve roles reinit when switching between sites

[johnjamesjacoby]

When switching between sites (since r39083) the $wp_roles global is being recreated instead of adjusting the values of the existing global.

The current regression means if a plugin was keeping its own copy of the $wp_roles global around (maybe as a property in a class, or for some other convenience sake) then then newly created $wp_roles global no longer points to that same copy. (See #23016 for more details & examples.)

History lesson: previous to that changeset, the reinit() method was used to update the roles for the switched site. That method was largely a copy of the _init() method, and was purpose built for blog switching.

---

Anecdotally, it's unlikely this will cause very many problems, but it is still a regression from previous behavior, and I believe the fix is fairly straightforward.

We can take inspiration from the WP_User::for_blog() method, and introduce a for_blog() method to the WP_Roles class to basically do the exact same thing in the exact same way. We can make alterations to the blog switching functions to update the $wp_roles and $current_user->roles values in unison.

Architecturally, I think it makes sense to call these methods together, but not have one call the other all the time, because there may be a reason or time where that control is welcome, and marrying them together now makes divorcing them later difficult.

Patch imminent.

[johnjamesjacoby]

(Edited typos and added a missing word.)

[mnelson4]

just because I was one of the nay-sayers about the previous implementation, just thought I should chime in saying this looks good! thanks @johnjamesjacoby

[nerrad]

I like this, I think it provides a clear way for client code to "properly" reset the WP_Roles instance if necessary. The only suggestion I would have is to rename the for_blog method to reset_for_blog or set_for_blog or init_for_blog. I think just for_blog is a bit ambiguous about the method purpose. However, I understand if for consistency sake with other places this similar naming schema is used, you keep it as is.

[johnjamesjacoby]

Thanks for looking! Yeah; I agree the for_blog() method name is no longer ideal.

In it's defense, nobody will need to interact with it directly pretty-much-ever. Once it's confirmed working and committed, it's a background process for keeping the environment variables in sync, and calling it directly is relatively advanced usage.

I'm ambivalent about the method name; I understand arguments for either/or. My bias would be towards predictability, even if it's predictably not ideal.

[knutsp]

Shouldn't this be for_site()?

[johnjamesjacoby]

Replying to knutsp:

Shouldn't this be for_site()?

See above. Unless we go back and rename WP_User::for_ blog(), it's hard to endorse anything else.

[flixos90]

Replying to johnjamesjacoby:

Unless we go back and rename WP_User::for_ blog(), it's hard to endorse anything else.

I agree with @knutsp that we should use for_site(). We shouldn't be locked in by the old naming conventions, so everything new should be properly named. Eventually we will be able to rename many of the old things over (long) time, but introducing new functionality in that way seems wrong to me.

Another issue that just came to mind is whether that function should exist in that form at all: Let's imagine WP_Roles is extended by a future WP_Network_Roles, what would a for_site() / for_blog() method do? It might be a better idea to find a more generic name.

[johnjamesjacoby]

Replying to flixos90:

I agree with @knutsp that we should use for_site(). We shouldn't be locked in by the old naming conventions, so everything new should be properly named. Eventually we will be able to rename many of the old things over (long) time, but introducing new functionality in that way seems wrong to me.

Then we need to rename WP_User::for_blog(). The inconsistency would be worse DUX than the mild inaccuracy, because anyone who does go digging will think for_blog and for_site do different things.

Another issue that just came to mind is whether that function should exist in that form at all: Let's imagine WP_Roles is extended by a future WP_Network_Roles, what would a for_site() / for_blog() method do? It might be a better idea to find a more generic name.

If we extend WP_Roles, it will still need some form of site-switching method, even at a global or network level.

Right now there is a regression to fix and existing approach to follow. I'd like to fix the problem without getting caught up on method names (which is worth scrutinizing in multisite, but it's a very dead horse by now.) If we want to rename things, we can do that at any time in the future.

[flixos90]

Replying to johnjamesjacoby:

Then we need to rename WP_User::for_blog(). The inconsistency would be worse DUX than the mild inaccuracy, because anyone who does go digging will think for_blog and for_site do different things.

We have a lot of such inconsistencies in multisite naming already (for example it's confusing that get_site_option() and get_network_option() both deal with network options). I think we should approach it in the way that we iteratively clean up the previous naming conventions in old code, thus not add any of these incorrect naming conventions in new code. Developers who work with Multisite are expected to be aware of these inconsistencies, and having clear doc blocks for functions/methods like these will help prevent confusion. Tickets like #36717 had that specific purpose.

Regarding this very issue, I opened a ticket to adjust that in #38681.

I think we slightly disagree here in the approach, so a third opinion would be much appreciated. I think this topic would make a good discussion for multisite office hours @jeremyfelt?

[jeremyfelt]

I'm going to move this to the 4.8 milestone as it has the early tag and seems like work meant to extend what was done in #23016 rather than a full realization of a regression that needs to be fixed in 4.7.

@johnjamesjacoby If that doesn't sound right, please yell. :)

[johnjamesjacoby]

No yelling here. Let's plan on this for 4.8, but be prepared for 4.7.1 if RC or GM drum-up any other unforeseen issues.

[flixos90]

Just a tiny bit related is #36961 - since that is bug, we should probably look at it at as well.

[flixos90]

38645.diff​ makes a few adjustments to the original patch and change for_blog() to for_site().

Since we're probably not going to do it (thus close #38681), I also created 38645.2.diff​ which only includes the adjustments without the function renaming. As discussed on WCUS contributor day, we'll likely introduce WP_Roles::for_blog() to have parity with the existing WP_User::for_blog() instead of renaming the existing function, therefore the latter patch. Its changes:

• do not remove the deprecated notice in WP_Roles::reinit(), but adjust it to point to WP_Roles::for_blog() (this was also causing a unit test to fail, but I think it makes sense to keep the method deprecated anyway)
• clarify docs for WP_Roles::for_blog() a bit

[flixos90]

38645.3.diff​ is the same as 38645.2.diff​ except that the did_action( 'init' ) check is included in switch_wp_roles(). It is currently being checked, so it should probably remain there.

[flixos90]

As discussed in today's bug scrub, this is looking pretty good. I'll review it in combination with #36961 in order to make sure we handle the changes in these tickets with parity.

[flixos90]

Let's come back to this early-4.9, in combination with #36961 - both these tickets require a thorough review.

[flixos90]

Milestoning for 4.9, together with #36961.

[flixos90]

38645.4.diff​ is an updated version of the patch with the primary goal of bringing it in line with the way WP_User works (see #36961 for the user-related ticket). Please have a look at particularly https://core.trac.wordpress.org/attachment/ticket/36961/36961.5.diff to see how the changes are in line with each other.

• In addition to the for_blog() method, a protected get_roles_data() method has been introduced that has retrieving the roles from the database as its sole responsibility. This method contains a fix for a bug that existed in the previous patches: When calling for_blog() with another ID than the current site, the options are not switched - in the latest patch the get_roles_data() method accounts for that. A test to verify this has been added as well. The WP_Roles::get_roles_data() method aligns with the WP_User::get_caps_data() method.
• A new init_roles() method has been introduced which fulfills the purpose of initializing the role variables based on the roles data that has been retrieved through get_roles_data(). The WP_Roles::init_roles() method aligns with the WP_User::get_role_caps() method - while they have different names, they both are responsible for setting up class properties based on the data retrieved from the DB.
• The _init() method has been deprecated. The code that used to be in it has been split out into for_blog() and its "sub-methods" get_roles_data() and init_roles(). for_blog() is a 1-to-1 replacement for the method.

Some of these changes may seem rather drastic, but they improve how switching sites works:

• By making the roles and user switching part a hook, developers can temporarily unhook it if they only want to switch options.
• A preliminary check for $use_db prevents unnecessary resets of the other role properties.
• The new for_blog() method allows for granular switching of only the roles, when it's needed. At the moment it still needs to call switch_to_blog() because of the option, but this technique opens up several possibilities in the future, for example a plugin could store the roles in blogmeta at some point, making the switch_to_blog() call entirely unnecessary for such a case. A real-world example that this could benefit even at this point can be seen in #36961: https://core.trac.wordpress.org/attachment/ticket/36961/36961.5.diff is based on the changes proposed here and simply calls wp_roles()->for_blog() instead of the bloated switch_to_blog().

Again, most of this is to be future-proof, but it already brings some benefits now.

[flixos90]

Note that the latest patch on #36961 introduces WP_User::for_site(), in favor of WP_User::for_blog(). If we make that change, the new method here should be called WP_Roles::for_site() as well. The original argument was to not introduce for_site() to maintain parity with WP_User, so if WP_User is now modernized, we should be good. :)

[jeremyfelt]

38645.4.diff​ is looking good.

• We're making the change to WP_User::for_site() in #36961, so we can do the same here.
• I have the same concerns in this ticket with removing the __call() and causing a fatal error for anyone using _init(). We can deprecate and leave the call in there, IMO.
• In get_roles_data(), remove_action() should only use 3 parameters.

I'd like to see feedback from @johnjamesjacoby on the latest changes. It'd also be good to hear from @mnelson4 and @charliespider, since they brought up issues on #23016 that helped lead to this ticket.

[mnelson4]

thumbs up from me! Technically leaving __call(), just deprecating it, would be more backwards compatible.

[flixos90]

38645.5.diff​ brings back the __call() method for BC, introduces a for_site() method (plus it calls all related variables $site_id) and removes the unnecessary parameter from the remove_action() call.

[flixos90]

Similar to the suggestions in #36961 for the WP_User class, 38645.6.diff​ introduces a WP_Roles::get_site_id() method to allow retrieving the ID of the site for which roles are currently initialized. Additional tests have been added to verify the new method works correctly, and an extra is_multisite() check has been added in WP_Roles::get_roles_data() to make sure that the get_blog_option() function is not called when not in multisite.

Adding needs-dev-note because of the deprecated WP_Roles::_init() method.

[flixos90]

In 41625:

Multisite: Improve initializing available roles when switch sites.

Switching the available roles and the current user's capabilities no longer happens in switch_to_blog() and restore_current_blog(), instead it has been moved to a new function wp_switch_roles_and_user() which is hooked into the site switching process. This allows to improve performance by temporarily unhooking the function when roles and capabilities do not need to be switched.

This change ensures that switching available roles now works closer to switching user capabilities, particularly the changes in [41624]. A new WP_Roles::for_site( $site_id ) method has been introduced, and the WP_Roles::_init() method has been deprecated. It is furthermore possible to retrieve the site ID for which the available roles are currently initialized through a new WP_Roles::get_site_id().

Props johnjamesjacoby, flixos90.
Fixes #38645.