Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy load user capabilities in WP_User object #5098

Open
wants to merge 17 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 53 additions & 10 deletions src/wp-includes/class-wp-user.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
* @property string $rich_editing
* @property string $syntax_highlighting
* @property string $use_ssl
* @property array $caps
* @property array $roles
* @property array $allcaps
*/
#[AllowDynamicProperties]
class WP_User {
Expand All @@ -62,7 +65,7 @@ class WP_User {
* @var bool[] Array of key/value pairs where keys represent a capability name
* and boolean values represent whether the user has that capability.
*/
public $caps = array();
protected $caps = array();

/**
* User metadata option name.
Expand All @@ -78,7 +81,7 @@ class WP_User {
* @since 2.0.0
* @var string[]
*/
public $roles = array();
protected $roles = array();

/**
* All capabilities the user has, including individual and role based.
Expand All @@ -87,7 +90,7 @@ class WP_User {
* @var bool[] Array of key/value pairs where keys represent a capability name
* and boolean values represent whether the user has that capability.
*/
public $allcaps = array();
protected $allcaps = array();

/**
* The filter context applied to user data fields.
Expand All @@ -105,6 +108,15 @@ class WP_User {
*/
private $site_id = 0;


/**
* Flag for if capability is loaded.
*
* @since 6.8.0
* @var bool
*/
private $loaded_caps = false;

/**
* @since 3.3.0
* @var array
Expand Down Expand Up @@ -287,6 +299,10 @@ public function __isset( $key ) {
$key = 'ID';
}

if ( in_array( $key, array( 'caps', 'allcaps', 'roles' ), true ) ) {
return isset( $this->$key );
}

if ( isset( $this->data->$key ) ) {
return true;
}
Expand Down Expand Up @@ -320,6 +336,11 @@ public function __get( $key ) {
return $this->ID;
}

if ( in_array( $key, array( 'caps', 'allcaps', 'roles' ), true ) ) {
$this->load_capability_data();
return $this->$key;
}

if ( isset( $this->data->$key ) ) {
$value = $this->data->$key;
} else {
Expand Down Expand Up @@ -514,6 +535,10 @@ public function get_role_caps() {

$wp_roles = wp_roles();

if ( ! $this->loaded_caps ) {
$this->caps = $this->get_caps_data();
}

Comment on lines +538 to +541
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary I think. The WP_User class has always populated $this->caps before, and not doing so anymore would be a breaking change. So we don't need this here, since $this->caps will always be correct prior to calling this method.

Suggested change
if ( ! $this->loaded_caps ) {
$this->caps = $this->get_caps_data();
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines below checks if caps is an array.

if ( is_array( $this->caps ) ) 

I wanted to ensure $this->caps was set correctly before doing this check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that that's unnecessary to check, because it'll always be set correctly based on $this->caps = $this->get_caps_data(), which is called before this get_role_caps() method everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method get_role_caps is public. What you create a WP_User instance and the call get_role_caps? It feels safer to just make sure that caps are loaded here. What does it hurt to have this here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's a fair point. Could you add an inline comment to clarify this? For example:

Suggested change
if ( ! $this->loaded_caps ) {
$this->caps = $this->get_caps_data();
}
// Edge-case: In case someone calls this method before lazy initialization, we need to initialize on demand.
if ( ! $this->loaded_caps ) {
$this->caps = $this->get_caps_data();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, ideally this method would also set $this->loaded_caps = true in that situation: If someone calls get_role_caps() before initialization, the data is all set up the same way as load_capability_data() does it. So there would be no point of lazy-loading that data again afterwards.

// Filter out caps that are not role names and assign to $this->roles.
if ( is_array( $this->caps ) ) {
$this->roles = array_filter( array_keys( $this->caps ), array( $wp_roles, 'is_role' ) );
Expand Down Expand Up @@ -547,6 +572,7 @@ public function add_role( $role ) {
if ( empty( $role ) ) {
return;
}
$this->load_capability_data();

if ( in_array( $role, $this->roles, true ) ) {
return;
Expand Down Expand Up @@ -576,6 +602,7 @@ public function add_role( $role ) {
* @param string $role Role name.
*/
public function remove_role( $role ) {
$this->load_capability_data();
if ( ! in_array( $role, $this->roles, true ) ) {
return;
}
Expand Down Expand Up @@ -608,6 +635,7 @@ public function remove_role( $role ) {
* @param string $role Role name.
*/
public function set_role( $role ) {
$this->load_capability_data();
if ( 1 === count( $this->roles ) && current( $this->roles ) === $role ) {
return;
}
Expand Down Expand Up @@ -709,6 +737,7 @@ public function update_user_level_from_caps() {
* @param bool $grant Whether to grant capability to user.
*/
public function add_cap( $cap, $grant = true ) {
$this->load_capability_data();
$this->caps[ $cap ] = $grant;
update_user_meta( $this->ID, $this->cap_key, $this->caps );
$this->get_role_caps();
Expand All @@ -723,6 +752,7 @@ public function add_cap( $cap, $grant = true ) {
* @param string $cap Capability name.
*/
public function remove_cap( $cap ) {
$this->load_capability_data();
if ( ! isset( $this->caps[ $cap ] ) ) {
return;
}
Expand All @@ -741,10 +771,10 @@ public function remove_cap( $cap ) {
*/
public function remove_all_caps() {
global $wpdb;
$this->caps = array();
delete_user_meta( $this->ID, $this->cap_key );
delete_user_meta( $this->ID, $wpdb->get_blog_prefix() . 'user_level' );
$this->get_role_caps();
$this->loaded_caps = false;
$this->load_capability_data();
}

/**
Expand Down Expand Up @@ -775,6 +805,8 @@ public function remove_all_caps() {
* the given capability for that object.
*/
public function has_cap( $cap, ...$args ) {
$this->load_capability_data();

if ( is_numeric( $cap ) ) {
_deprecated_argument( __FUNCTION__, '2.0.0', __( 'Usage of user levels is deprecated. Use capabilities instead.' ) );
$cap = $this->translate_level_to_cap( $cap );
Expand Down Expand Up @@ -875,11 +907,8 @@ public function for_site( $site_id = '' ) {
$this->site_id = get_current_blog_id();
}

$this->cap_key = $wpdb->get_blog_prefix( $this->site_id ) . 'capabilities';

$this->caps = $this->get_caps_data();

$this->get_role_caps();
$this->cap_key = $wpdb->get_blog_prefix( $this->site_id ) . 'capabilities';
$this->loaded_caps = false;
}

/**
Expand Down Expand Up @@ -910,4 +939,18 @@ private function get_caps_data() {

return $caps;
}

/**
* Loads capability data if it has not been loaded yet.
*
* @since 6.8.0
*/
private function load_capability_data() {
if ( $this->loaded_caps ) {
return;
}
$this->caps = $this->get_caps_data();
$this->get_role_caps();
$this->loaded_caps = true;
}
}
35 changes: 35 additions & 0 deletions tests/phpunit/tests/user/capabilities.php
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,41 @@ public function test_role_remove_cap() {
$this->assertFalse( $wp_roles->is_role( $role_name ) );
}

/**
* @ticket 58001
*/
public function test_get_role_caps() {
$id_1 = self::$users['contributor']->ID;
$user_1 = new WP_User( $id_1 );

$role_caps = $user_1->get_role_caps();
$this->assertIsArray( $role_caps, 'User role capabilities should be an array' );
$this->assertArrayHasKey( 'edit_posts', $role_caps, 'User role capabilities should contain the edit_posts capability' );
}

/**
* @ticket 58001
*/
public function test_user_lazy_capabilities() {
$id_1 = self::$users['contributor']->ID;
$user_1 = new WP_User( $id_1 );

$this->assertTrue( isset( $user_1->roles ), 'User roles should be set' );
$this->assertTrue( isset( $user_1->allcaps ), 'User all capabilities should be set' );
$this->assertTrue( isset( $user_1->caps ), 'User capabilities should be set' );
$this->assertIsArray( $user_1->roles, 'User roles should be an array' );
$this->assertSame( array( 'contributor' ), $user_1->roles, 'User roles should match' );
$this->assertIsArray( $user_1->allcaps, 'User allcaps should be an array' );
$this->assertIsArray( $user_1->caps, 'User caps should be an array' );

$caps = $this->getAllCapsAndRoles();
foreach ( $caps as $cap => $roles ) {
if ( in_array( 'contributor', $roles, true ) ) {
$this->assertTrue( $user_1->has_cap( $cap ), "User should have the {$cap} capability" );
}
}
}

/**
* Add an extra capability to a user.
*/
Expand Down
3 changes: 2 additions & 1 deletion tests/phpunit/tests/user/multisite.php
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,13 @@ public function test_add_user_to_blog_subscriber() {

switch_to_blog( $site_id );
$user = get_user_by( 'id', $user_id );
$this->assertContains( 'subscriber', $user->roles, 'User should have subscriber role' );
restore_current_blog();

wp_delete_site( $site_id );
wpmu_delete_user( $user_id );

$this->assertContains( 'subscriber', $user->roles );
$this->assertContains( 'subscriber', $user->roles, 'User should still have subscriber role' );
}

/**
Expand Down
7 changes: 6 additions & 1 deletion tests/phpunit/tests/user/query.php
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,18 @@ public function test_get_all_primed_users() {
$filter = new MockAction();
add_filter( 'update_user_metadata_cache', array( $filter, 'filter' ), 10, 2 );

new WP_User_Query(
$query = new WP_User_Query(
array(
'include' => self::$author_ids,
'fields' => 'all',
)
);

$users = $query->get_results();
foreach ( $users as $user ) {
$user->roles;
}

$args = $filter->get_args();
$last_args = end( $args );
$this->assertIsArray( $last_args[1] );
Expand Down
Loading