WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#16617 closed enhancement (invalid)

New WP_Roles

Reported by: jrfoell Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0.5
Component: Role/Capability Keywords:
Focuses: Cc:

Description

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.

Attachments (6)

add_cap_oo.zip (1.2 KB) - added by jrfoell 5 years ago.
add_cap_procedural.zip (1.1 KB) - added by jrfoell 5 years ago.
plugin.php (1021 bytes) - added by jrfoell 5 years ago.
OO Plugin
plugin.2.php (736 bytes) - added by jrfoell 5 years ago.
Procedural plugin
get_role_diff.patch (1.1 KB) - added by jrfoell 5 years ago.
Static WP_Roles::get_role() patch
capabilities_patch.diff (608 bytes) - added by jrfoell 5 years ago.

Download all attachments as: .zip

Change History (25)

@jrfoell5 years ago

@jrfoell5 years ago

comment:1 @dd325 years ago

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?

@jrfoell5 years ago

OO Plugin

@jrfoell5 years ago

Procedural plugin

comment:2 @jrfoell5 years ago

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

comment:3 @jrfoell5 years ago

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

comment:4 @scribu5 years ago

  • Cc scribu added

comment:5 @solarissmoke5 years ago

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?

Last edited 5 years ago by solarissmoke (previous) (diff)

comment:6 @solarissmoke5 years ago

  • Keywords close added

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' );
}

@jrfoell5 years ago

Static WP_Roles::get_role() patch

comment:7 @jrfoell5 years ago

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.

Last edited 5 years ago by jrfoell (previous) (diff)

@jrfoell5 years ago

comment:8 @jrfoell5 years ago

  • Keywords has-patch added; close removed
  • Version changed from 3.0.5 to 3.1

comment:9 @sivel5 years ago

  • Version changed from 3.1 to 3.0.5

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

comment:10 @jrfoell5 years ago

Sorry, the patch was generated and tested against 3.1 today

comment:11 @jrfoell4 years ago

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

comment:12 @nacin4 years ago

  • Keywords close added
  • Summary changed from Bulk activate plugins: add capabilities with WP_Roles::add_cap() fails, while procedural add_cap() does not to New WP_Roles
  • Type changed from defect (bug) to enhancement

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.

comment:13 @scribu4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

What nacin said.

Correct usage is plugin.2.php.

comment:14 @jrfoell4 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

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.

Last edited 4 years ago by jrfoell (previous) (diff)

comment:15 @scribu4 years ago

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?

comment:16 @scribu4 years ago

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

comment:17 @nacin4 years ago

  • Resolution set to invalid
  • Status changed from reopened to closed

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

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

comment:18 @jrfoell4 years ago

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

Last edited 4 years ago by jrfoell (previous) (diff)

comment:19 @scribu4 years ago

  • Keywords has-patch close removed

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.

Note: See TracTickets for help on using tickets.