Allow plugins to manipulate WP_Roles ($wp_roles global) on construct

[johnjamesjacoby]

The WP_Roles class is difficult to extend. There are no actions, filters, or intercept points to allow plugins to target it. Because the $wp_roles global can be created anytime (even doing_it_wrong on plugins_loaded) there's no reliable way to modify any part of $wp_roles.

(The only place WP_Roles actually has any action at all is in the set_role() method, which is the least useful function in the class since it noops all user roles if a user has more than one.)

The specific use case I have is bbPress's dynamic roles.

bbPress is unable to add its dynamic roles to the $wp_roles global on switch_to_blog(). Neither of WP_Roles's initialization methods have actions to allow bbPress to add its roles on.

• switch_to_blog() calls $wp_roles->reinit()
• then wp_get_current_user()
• finally $current_user->for_blog()

At no point can bbPress reliably append its roles to $wp_roles->roles, causing the current user to incorrectly be missing their bbPress role for that site.

More over, the WP_Roles::reinit() method seems like a one-use stop-gap to fix a specific problem, that ends up introducing other issues with trying to extend it. If the use_db flag is set to false, it bails early, and there's no way for plugins that don't use database roles to reinitialize their roles correctly again.

The attached patch does two things:

• Removes the reinit() method completely, and instead re-instantiates the $wp_roles global inside switch_to_blog(). WP_Roles::_init() is not a heavy function, and the guts are basically identical between the two methods.
• Adds a byref action to WP_Roles::init() that allow plugins like bbPress to add their dynamic roles anytime $wp_roles is created.

[TobiasBg]

I recently stumbled upon a problem with WP_Roles, as well, when the user's caps where not refreshed after adding a cap to the user's role. See

​http://lists.automattic.com/pipermail/wp-hackers/2012-December/044927.html
​http://lists.automattic.com/pipermail/wp-hackers/2012-December/044933.html

I ended up with refreshing this manually, although WP_Roles should probably take care of this by itself:

// Add or remove caps
...->add_cap( ... );
// Refresh current set of capabilities of the user, to be able to directly use the new caps
$user = wp_get_current_user();
$user->get_role_caps();

[JustinSainton]

I concede to being generally daft, but is passing by reference necessary here? Didn't think this was necessary any longer in PHP5?

See: ​http://stackoverflow.com/questions/9331519/php-object-by-reference-in-php5/9332219#9332219.

Definitely open to being wrong though. Always learning something new :)

[knutsp]

Replying to JustinSainton:

I concede to being generally daft, but is passing by reference necessary here? Didn't think this was necessary any longer in PHP5?

Call time pass by reference is not only not necessary in PHP 5, it was deprecated in PHP 5.3 and removed in PHP 5.4, then resulting in a fatal error.

[johnjamesjacoby]

It's done similarly 19 times in WordPress core; I maintained the current status quo here.

I originally patched without the byref, ironically enough -- I went back to put it in to match. :)

[JustinSainton]

Related: #16752, #21839

[johnjamesjacoby]

See: r21485, ​#BB2452

[jbermudez]

I'd love to see @johnjamesjacoby's changeset merged so that I can use bbpress in a multisite instance. I would think that any plugin that assigns dynamic roles would be in trouble on a multisite instance due to the way switch_to_blog wipes out caps.

[pento]

Per discussion in ticket:29783:53, this probably needs to be fixed for 4.7.

[pento]

In 23016.diff​:

• Instead of removing WP_Roles::reinit(), deprecate it and point it to WP_Roles::_init(), instead.
• Add documentation for wp_roles_init.
• Duplicate the $wp_roles = new WP_Roles() behaviour in restore_current_blog().
• Add unit tests.

@johnjamesjacoby, @netweb: Before committing this, I'd like to see a sample implementation in bbPress, to ensure it does the job.

[netweb]

Replying to pento:

@johnjamesjacoby, @netweb: Before committing this, I'd like to see a sample implementation in bbPress, to ensure it does the job.

@pento I'll leave @johnjamesjacoby to this now that he's created ​#BuddyPress7305, expect a bbPress ticket to follow

[pento]

I'm happy to leave this ticket until beta 2 (due to it affecting bbPress), but it needs some soak time, so I'm not comfortable leaving it any longer than that.

@johnjamesjacoby - sorry to be putting a deadline on you, but I need you to do testing before then.

[DrewAPicture]

Holding off on #36341 pending changes here.

[johnjamesjacoby]

Refresh of 23016.diff

[johnjamesjacoby]

@pento patch refreshed against latest trunk, and it works exactly as I intended for it to (and how bbPress really benefits from.)

WordPress tests are passing, and I can add new tests to bbPress to support this.

Related bbPress commits to trunk: ​[BB6106], ​[BB6107].

Please commit at your leisure.

---

Related: it's probably worth a post on Make to highlight this new action, along with the current-user load-order change that's coming with user languages.

[jorbin]

@pento can you review this please.

[pento]

Thanks for the testing and patch refresh, @johnjamesjacoby!

I agree, a dev note would be valuable for this.

[pento]

In 39082:

Roles/Capabilities: Add a new wp_roles_init filter.

Historically, it's been difficult to extend user roles, but reasonable to work around by waiting until after init has fired, to add custom roles and capabilities. With the addition of Locale Switching, Core now potentially loads roles before init has fired, leaving a window where custom roles and capabilities are not handled.

The new filter allows plugins to add their own custom roles whenever they're initialised (on page load, or when switching sites, for example), so that they can always be obeyed.

WP_Roles has also been tidied up a little bit, to remove duplicate code.

Props johnjamesjacoby, pento.
Fixes #23016.

[johnbillion]

@pento The @param documentation for the wp_roles_init filter is missing the parameter name.

[pento]

In 39083:

Docs: Add the parameter name for the wp_roles_init action.

[39082] missed adding the name of the parameter to the docs of the wp_roles_init action.

Props johnbillion for the catch.
See #23016.

[johnjamesjacoby]

Alright! Thanks everyone!

[mnelson4]

new WP_Roles() isn't an exact replacement for $wp_roles->reinit() because it returns a NEW WP_Roles object, whereas $wp_roles->reinit() returned the same old WP_Roles object.

So if code anywhere stores a reference to the global $wp_roles object, it can easily become stale if other code anywhere else changes that global in order to reset roles.

Eg

global $wp_roles;
$my_reference_to_wp_roles = $wp_roles;
$wp_roles = new WP_Roles();

Now $my_reference_to_wp_roles still points to the old WP_Roles object, not the new WP_Roles object we just created, and I'll get into trouble if I use it.

Whereas using the now-deprecated code

global $wp_roles;
$my_reference_to_wp_roles = $wp_roles;
$wp_roles->reinit();

doesn't have this problem: $my_reference_to_wp_roles stills points to the same thing as$wp_roles, so I can continue to use $my_reference_to_wp_roles just fine.

Granted, before these changes, plugin or theme code could have switched $wp_roles and created this problem. But now the *recommended* way of resetting $wp_roles is to do $wp_roles = new WP_Roles(), which can cause problems for any code who holds a reference to the old global before it got changed.

Maybe we could say "Well, if you ever created a variable referencing the global $wp_roles, you were doing_it_wrong()`, but is that mentioned in documentation anywhere? If so, ya then they're doing it wrong and its their problem. If not, we should probably be a bit more accommodating.

So I think these changes are fine, except I don't think deprecating WP_Roles::reinit() is good, and we should continue to use WP_Roles::reinit() instead of calling $wp_roles = new WP_Roles().

Thoughts?

[johnjamesjacoby]

@mnelson4 - interesting thoughts. I think the original behavior was unintentional, and problematic for the way WordPress (and plugins & themes) interact with Roles.

The old reinit() method used $this vs. directly touching the $wp_roles global, so the current _init() method is a direct equivalent -- either one could be used for the same end result (though, I think I found a different consequence.)

Please consider these (very crude) examples (that should probably be unit tests):

add_action( 'init', function() {
	$wp_roles                 = new WP_Roles();
	$my_reference_to_wp_roles = $wp_roles;
	$wp_roles->foo            = 'bar';

	var_dump( $wp_roles->foo );
	var_dump( $my_reference_to_wp_roles->foo );

	die;
} );

Will output:

string 'bar' (length=3)
string 'bar' (length=3)

$my_reference_to_wp_roles was never modified, but because the variable it points to was, it was too.

---

The same happens if you use the global:

add_action( 'init', function() {
	global $wp_roles;

	$my_reference_to_wp_roles = $wp_roles;
	$wp_roles->foo            = 'bar';

	var_dump( $wp_roles->foo );
	var_dump( $my_reference_to_wp_roles->foo );

	die;
} );

Will output:

string 'bar' (length=3)
string 'bar' (length=3)

---

If we intentionally try to wipe out $wp_roles, we can. Our clone keeps the changes (stale compared to the global.)

add_action( 'init', function() {
	global $wp_roles;

	$my_reference_to_wp_roles = $wp_roles;
	$wp_roles->foo            = 'bar';

	$wp_roles = $wp_roles->reinit();
	var_dump( $wp_roles->foo );

	$wp_roles->foo = 'bar';
	$wp_roles = new WP_Roles();

	var_dump( $wp_roles->foo );
	var_dump( $my_reference_to_wp_roles->foo );

	die;
} );

Will output:

string null
string null
string 'bar' (length=3)

---

Lastly, let's mix & match a changed global reference:

add_action( 'init', function() {
	global $wp_roles, $new_roles;

	$new_roles = $wp_roles;

	$wp_roles->changed = true;
}, 10 );

add_action( 'init', function() {
	global $wp_roles;
	$wp_roles->changed = false;
}, 11 );

add_action( 'init', function() {
	global $wp_roles, $new_roles;

	var_dump( $wp_roles->changed );
	var_dump( $new_roles->changed );
	
	die;
}, 12 );

And we'll get...

boolean false
boolean false

---

There is also wp_roles(), which (in)conveniently obscured the problem. There might be places (even in core) that would benefit from using wp_roles() vs new WP_Roles(), but we'll need to audit those & make decisions based on intent. Basically: yield or not yield. (At a cursory, I think all usages are OK.)

Either way, it appears to my eyes that there's not much of a change here, and what change there is creates a more predictable and easier to understand interaction with WP_Roles.

---

I think that WP_Roles->reinit() does need to keep it's use_db check, though. Otherwise the $wp_user_roles isn't going to be respected as the omega roles array for the entire installation. Patch imminent for this.

[johnjamesjacoby]

Restore use_db check in reinit(), removed in r39082.

[charliespider]

@johnjamesjacoby 

I think you missed the point @mnelson4 was trying to make.

Your examples seem to just be illustrating how objects are passed by reference in PHP (ie: variables are pointers), but without addressing the issue that $wp_roles->reinit() NOW creates a NEW object (ie: new pointer to new object), whereas it previously only affected the existing global object (existing pointer to existing object).

previously:

<?php
global $wp_roles; // points to object with spl_object_hash of "A" (massive simplification)
$my_reference_to_wp_roles = $wp_roles; // also points to object with spl_object_hash of "A"
$wp_roles->reinit(); 
// $wp_roles STILL points to object with spl_object_hash of "A"
// $my_reference_to_wp_roles also STILL points to object with spl_object_hash of "A"

whereas now:

<?php
global $wp_roles; // points to object with spl_object_hash of "A" (massive simplification)
$my_reference_to_wp_roles = $wp_roles; // also points to object with spl_object_hash of "A"
$wp_roles = new WP_Roles(); // NOW points to object with spl_object_hash of "B" (or C, or anything but A)

in the latter example, we now have pointers to TWO WP_Roles objects floating around:

• A local variable pointing to object A (the original $wp_roles global)
• and object B which is a new object that has replaced the global.

[ edited because I had erringly stated that $wp_roles->reinit() returned an object, but it does not ]

[mnelson4]

adds to the unit test for 23016 branch that shows a potential problem if a variable is created referencing the $wp_roles global

[mnelson4]

@johnjamesjacoby I'm not sure you got my point: $wp_roles->reinit() shouldn't be replaced by $wp_roles = new WP_Roles() like the patches and deprecation notice on WP_Roles::reinit() indicate.

See

• https://core.trac.wordpress.org/attachment/ticket/23016/23016.02.diff, line 148 of the new file for the deprecation notice
• https://core.trac.wordpress.org/attachment/ticket/23016/23016.patch, wp-includes/ms-blogs.php line 545 for where $wp_roles->reinit() was replaced by $wp_roles = new WP_Roles()
• https://core.trac.wordpress.org/attachment/ticket/23016/23016.diff, src/wp-includes/ms-blogs.php lines 824, 898, tests/phpunit/tests/user/capabilities.php line 35 for more spots where $wp_roles->reinit() was replaced by $wp_roles = new WP_Roles()

The patch I uploaded adds to your unit test, and I hope shows the problem. (Note that unit test fails on master branch, whereas adding that unit test to the 4.5.2 tag it passes).

[johnjamesjacoby]

Thanks @charliespider for chiming in, and @mnelson4 for the patch & examples.

I think I missed the point because the behavior of $wp_roles->reinit() (and/or _init()) itself hasn't changed. Sorry for the confirmation bias, there.

I fully understand what the issue is now. Did either of you run into problems in the wild with this? bbPress didn't break, and it pushes the envelope pretty far with roles, but it also wasn't working as reliably as it should have (and will now thanks to the wp_roles_init action.)

If it is a problem, we need to un-deprecate reinit() & revert back to calling it, but the way I've patched it in 23016.03.patch instead. $wp_roles = new WP_Roles() will always result in a new hash ID, and the reinit() method can prevent that if that's what we want/need.

---

Originally I had thought to approach this by adding a for_blog() method to WP_Roles like WP_User has, and hooking it to switch_blog, but remembered that's what both reinit() and _init() already mostly did, so went the route of removing duplication instead of adding to it. I have a hunch my original hunch is a more complete & appropriate fix, though... it's likely more invasive than anyone is comfortable with during beta, and those boundaries are in place for good reason.

I started working on a patch tonight, but am running out of steam and can have it ready tomorrow for scrutiny.

[pento]

Thanks for the feedback, @mnelson4 and @charliespider!

@johnjamesjacoby: When you have a patch ready, could you create a new ticket, please?

[johnjamesjacoby]

Will do @pento.

[charliespider]

Did either of you run into problems in the wild with this?

No not this specifically, but we have been bitten by creating local variables from globally scoped objects and then ended up with duplicates after the global was changed.

So we basically just want to help prevent you from falling into the same hole we have. It's not an easy situation to fix, and one of the reasons why globals should be avoided like the plague.

If PHP allowed objects to actually be immediately destroyed when you unset() them, then this would be easy, because you could simply kill the old object, replace the global, and any local copies of the old global would be rekt. But unfortunately, unset objects just stick around, allowing any code with local copies to still access and use them, up until PHP needs more memory and they get garbage collected.

The only way around it that I can think of is to use an internal property to mark the global object as active, and then set that to false on the old object when replacing the global. Then you can issue a doing_it_wrong() if the object is used. Pretty fugly hack, but what else can you do when you've already given an object global scope in a distributed product?

...actually... i can think of some other fugly hacks, but they just get even crazier than the one above... I scare myself sometimes...

[mnelson4]

+1 to all the above!

[johnjamesjacoby]

Created #38645 to outline details & attached a patch.

[desrosj]

This was detailed in the following dev note: ​https://make.wordpress.org/core/2016/11/07/changed-loading-order-for-current-user-in-4-7/.