WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#23016 closed defect (bug) (fixed)

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

Reported by: johnjamesjacoby Owned by: pento
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.5
Component: Role/Capability Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: Cc:
PR Number:

Description

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.

Attachments (5)

23016.patch (2.0 KB) - added by johnjamesjacoby 7 years ago.
23016.diff (5.4 KB) - added by pento 3 years ago.
23016.02.diff (5.2 KB) - added by johnjamesjacoby 3 years ago.
Refresh of 23016.diff
23016.03.patch (392 bytes) - added by johnjamesjacoby 3 years ago.
Restore use_db check in reinit(), removed in r39082.
23016-shows-problem.patch (1.3 KB) - added by mnelson4 3 years ago.
adds to the unit test for 23016 branch that shows a potential problem if a variable is created referencing the $wp_roles global

Download all attachments as: .zip

Change History (42)

#1 @TobiasBg
7 years ago

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();

#2 @scribu
7 years ago

  • Cc scribu added

#3 @knutsp
7 years ago

  • Cc knut@… added

#4 follow-up: @JustinSainton
7 years ago

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 :)

#5 in reply to: ↑ 4 @knutsp
7 years ago

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.

#6 @johnjamesjacoby
7 years ago

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. :)

#8 @ryan
6 years ago

  • Milestone changed from 3.6 to Future Release

#10 @jbermudez
6 years ago

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.

#11 @knutsp
4 years ago

  • Keywords dev-feedback added; 3.6-early removed

This ticket was mentioned in Slack in #core-multisite by jjj. View the logs.


3 years ago

#13 @pento
3 years ago

  • Keywords has-patch needs-refresh needs-unit-tests added; early removed
  • Milestone changed from Future Release to 4.7

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

Last edited 3 years ago by pento (previous) (diff)

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

@pento
3 years ago

#16 follow-up: @pento
3 years ago

  • Keywords reporter-feedback needs-testing has-unit-tests added; dev-feedback needs-refresh needs-unit-tests removed

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.

#17 in reply to: ↑ 16 @netweb
3 years ago

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

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

#19 @pento
3 years ago

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.

#20 @DrewAPicture
3 years ago

Holding off on #36341 pending changes here.

@johnjamesjacoby
3 years ago

Refresh of 23016.diff

#21 @johnjamesjacoby
3 years ago

  • Keywords commit added; reporter-feedback needs-testing removed

@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.

#22 @jorbin
3 years ago

  • Owner set to pento
  • Status changed from new to assigned

@pento can you review this please.

#23 @pento
3 years ago

  • Keywords needs-dev-note added
  • Status changed from assigned to accepted

Thanks for the testing and patch refresh, @johnjamesjacoby!

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

#24 @pento
3 years ago

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

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.

#25 @johnbillion
3 years ago

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

#26 @pento
3 years ago

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.

#27 @johnjamesjacoby
3 years ago

Alright! Thanks everyone!

#28 @mnelson4
3 years ago

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?

#29 @johnjamesjacoby
3 years ago

@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
3 years ago

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

#30 @charliespider
3 years ago

@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 ]

Last edited 3 years ago by charliespider (previous) (diff)

@mnelson4
3 years ago

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

#31 @mnelson4
3 years ago

@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

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).

#32 @johnjamesjacoby
3 years ago

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.

#33 @pento
3 years ago

Thanks for the feedback, @mnelson4 and @charliespider!

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

#34 @johnjamesjacoby
3 years ago

Will do @pento.

#35 @charliespider
3 years ago

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...

Last edited 3 years ago by charliespider (previous) (diff)

#36 @mnelson4
3 years ago

+1 to all the above!

#37 @johnjamesjacoby
3 years ago

Created #38645 to outline details & attached a patch.

Note: See TracTickets for help on using tickets.