#38645 closed defect (bug) (fixed)
Improve roles reinit when switching between sites
Reported by: | johnjamesjacoby | Owned by: | flixos90 |
---|---|---|---|
Milestone: | 4.9 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | Role/Capability | Keywords: | has-patch, has-unit-tests |
Focuses: | multisite | Cc: |
Description (last modified by )
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.
Attachments (7)
Change History (40)
#2
@
8 years ago
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
#3
@
8 years ago
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.
#4
@
8 years ago
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.
This ticket was mentioned in Slack in #bbpress by netweb. View the logs.
8 years ago
#7
in reply to:
↑ 6
;
follow-up:
↓ 8
@
8 years ago
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.
#8
in reply to:
↑ 7
;
follow-up:
↓ 9
@
8 years ago
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.
#9
in reply to:
↑ 8
;
follow-up:
↓ 10
@
8 years ago
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 futureWP_Network_Roles
, what would afor_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.
#10
in reply to:
↑ 9
@
8 years ago
- Keywords 2nd-opinion added
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?
#11
@
8 years ago
- Keywords early removed
- Milestone changed from Awaiting Review to 4.8
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. :)
This ticket was mentioned in Slack in #core-multisite by jjj. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#14
@
8 years ago
- Keywords 2nd-opinion removed
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.
#15
@
8 years ago
Just a tiny bit related is #36961 - since that is bug, we should probably look at it at as well.
#16
@
8 years ago
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 toWP_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
#17
@
8 years ago
- Keywords needs-testing removed
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.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
8 years ago
#19
@
8 years ago
- Owner set to flixos90
- Status changed from new to reviewing
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.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
8 years ago
#21
@
8 years ago
- Keywords early added
- Milestone changed from 4.8 to Future Release
Let's come back to this early-4.9, in combination with #36961 - both these tickets require a thorough review.
#22
@
7 years ago
- Keywords early removed
- Milestone changed from Future Release to 4.9
Milestoning for 4.9, together with #36961.
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
7 years ago
#24
@
7 years ago
- Keywords has-unit-tests added
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 protectedget_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 callingfor_blog()
with another ID than the current site, the options are not switched - in the latest patch theget_roles_data()
method accounts for that. A test to verify this has been added as well. TheWP_Roles::get_roles_data()
method aligns with theWP_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 throughget_roles_data()
. TheWP_Roles::init_roles()
method aligns with theWP_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 intofor_blog()
and its "sub-methods"get_roles_data()
andinit_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 callswitch_to_blog()
because of the option, but this technique opens up several possibilities in the future, for example a plugin could store the roles inblogmeta
at some point, making theswitch_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 callswp_roles()->for_blog()
instead of the bloatedswitch_to_blog()
.
Again, most of this is to be future-proof, but it already brings some benefits now.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
#26
@
7 years ago
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. :)
#27
@
7 years ago
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.
#28
@
7 years ago
thumbs up from me! Technically leaving __call()
, just deprecating it, would be more backwards compatible.
#29
@
7 years ago
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.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
#31
@
7 years ago
- Keywords needs-dev-note added
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.
(Edited typos and added a missing word.)