New WP_Roles

[jrfoell]

Attached two example plugins, one procedural, one object oriented. Both add a test capability to the administrator role and check for it (printing in the footer). If you activate them individually, they're both fine, if you activate them both at the same time, the OO one fails.

[dd32]

Could you attach the individual PHP files? Easier for people to glance over.

Have you tried 2 procedural plugins?

How did you discover this not working?

Which version of PHP?

[jrfoell]

OO Plugin

[jrfoell]

Procedural plugin

[jrfoell]

I discovered it when trying to activate two newly installed plugins that used the different role methods. I activated them both at the same time (using the bulk activate feature) and noticed the menus for one plugin wouldn't show up unless I deactivated it, then reactivated it by itself.

Created some example plugins to make testing simpler.

PHP version: 5.3.3

[jrfoell]

I duplicated the procedural plugin and bulk activating those two was fine.

[solarissmoke]

Here's the problem:

The function get_role() uses the global $wp_roles, which is instantiated the first time that one of the capability functions are called. At instantiation, it loads the wp_user_roles option and stores it as $wp_roles->roles.

Now, whenever you use $wp_roles::add_cap() (as the procedural plugin does), it simply adds the capability to $wp_roles->roles, and then does update_option() to store it in the database. If, in the mean time, something else has updated the wp_user_roles option independently (which is what the OO plugin does, with it's own instatiation of the WP_roles class), then $wp_roles has no knowledge of this and simply overwrites those changes. Hence the capability added by the OO plugin is overwritten.

Basically, the current setup only works if you have *one* instantiation of the WP_Roles class. Things will break if plugin authors create their own - because the wp_user_roles option is loaded only once, at instantiation. Having two instances of the class making concurrent changes will always cause one set to be overwritten by the other.

I'm no expert, but I can't see why you would want more than one instance of WP_Roles anyway?

[solarissmoke]

I think this is a case of incorrect usage. The OO plugin functions should do exactly what the procedural ones do - i.e., let wordpress handle it:

public function onActivation() {
	$role = get_role( 'administrator' );
	$role->add_cap( 'add_cap_oo' );
}

public function onDeactivation() {
	$role = get_role( 'administrator' );
	$role->remove_cap( 'add_cap_oo' );
}

[jrfoell]

Static WP_Roles::get_role() patch

[jrfoell]

Ignore the static patch to get_role... that is not a good solution because you can't prevent the constructor of WP_Roles from being public in PHP4. However, having WP_Roles' $roles property use the same reference as $wp_user_roles will work.

I have attached a change that will allow both methods to work as normal.

[sivel]

Please do not change the version specified from the original ticket creation. We use this to track when the bug was initially noticed/introduced.

[jrfoell]

Sorry, the patch was generated and tested against 3.1 today

[jrfoell]

Can someone please look at capabilities_patch.diff to see if it can be added to 3.1.2?

[nacin]

WP_Roles is designed to function as a de facto singleton. The global $wp_roles should hold the only instantiation of the WP_Roles object. While there might be a use case for instantiating it a second time, it would be invalid to attempt to do so when trying to modify the roles definition in the database.

$wp_user_roles exists for a specific reason -- to allows the DB to be bypassed. The use case is defining the roles definition in, say, wp-config.php. Good for multisites with custom roles/caps. I think the attached patch, while might not have any side effects, really isn't how it's designed.

WP_Roles has never worked the way you've described. I'd think the proper way to handle this ticket would be to mark as invalid on that basis.

[scribu]

What nacin said.

Correct usage is plugin.2.php​.

[jrfoell]

C'mon... did you even look at the patch?!?

Why provide a WP_Roles object if it's not to be used?

Edit: I missed Nacin's response, but I'd still like this fix to be considered for a future release.

[scribu]

Yes, we looked at the patch.

Not all objects are meant to be instantiated from plugins.

If WP were written directly in PHP5, WP_Roles would be an abstract class, a singleton, as nacin mentioned.

The more important question is why you think you need to do this. What's the benefit?

[scribu]

We could ask a similar question: did you even read the comments that various people left here?

[nacin]

It's not a fix. You are using the class wrong.

You can use the $wp_roles object. Not the WP_Roles class.

[jrfoell]

scribu, I did read the comments, and edited my original response to reflect that.

While allowing the instantiation of a new WP_Roles is probably not needed, my complaint remains. The usage of objects (or restriction of) in WordPress is unclear. On one hand it seems like the code base is trying to modernize by providing these objects, but on the other hand you're telling me not to use them. Since WP is still supporting PHP4, the documentation should at least reflect the intended visibility of the class.

Would you accept a patch if it were merely to state that WP_Role's constructor is intended to be (mostly) private, called once as a singleton?

I found this code in an existing plugin, and it seemed like perfectly legitimate usage. I have since changed it to use get_role().

[scribu]

Would you accept a patch if it were merely to state that WP_Role's constructor is intended to be (mostly) private, called once as a singleton?

That sounds like a good idea.