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.

Sign up for GitHub

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 11 commits into
base: trunk
Choose a base branch
from
Open

Lazy load user capabilities in WP_User object #5098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a02f807
Test
spacedmonkey Aug 26, 2023
06cbd09
Add wp_lazyload_user_meta function and use it.
spacedmonkey Aug 26, 2023
8046889
Fix tests.
spacedmonkey Aug 26, 2023
43d8eb3
Merge branch 'trunk' into try/lazy-load
spacedmonkey Jul 1, 2024
c95b154
Reduce number of expected queries in cacheResults test
spacedmonkey Jul 1, 2024
ba07a49
Merge branch 'trunk' into try/lazy-load
spacedmonkey Nov 17, 2024
ab9fd92
Hopefully fix multisite tests.
spacedmonkey Jan 30, 2025
623d977
Merge branch 'trunk' into try/lazy-load
spacedmonkey Jan 30, 2025
64cefd6
Force reload on remove_all_caps.
spacedmonkey Jan 30, 2025
719028e
Improve test.
spacedmonkey Jan 31, 2025
10df03b
Update src/wp-includes/user.php
spacedmonkey Jan 31, 2025
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
4 changes: 4 additions & 0 deletions src/wp-includes/class-wp-metadata-lazyloader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ public function __construct() {
'filter' => 'get_blog_metadata',
'callback' => array( $this, 'lazyload_meta_callback' ),
),
'user' => array(
'filter' => 'get_user_metadata',
'callback' => array( $this, 'lazyload_meta_callback' ),
),
);
}

Expand Down
59 changes: 51 additions & 8 deletions src/wp-includes/class-wp-user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,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();
felixarntz marked this conversation as resolved.
Show resolved Hide resolved

/**
* User metadata option name.
Expand All @@ -77,7 +77,7 @@ class WP_User {
* @since 2.0.0
* @var string[]
*/
public $roles = array();
protected $roles = array();
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved

/**
* All capabilities the user has, including individual and role based.
Expand All @@ -86,7 +86,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();
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved

/**
* The filter context applied to user data fields.
Expand All @@ -104,6 +104,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 @@ -286,6 +295,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 @@ -319,6 +332,11 @@ public function __get( $key ) {
return $this->ID;
}

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

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

$wp_roles = wp_roles();

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

// 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 @@ -546,6 +568,7 @@ public function add_role( $role ) {
if ( empty( $role ) ) {
return;
}
$this->load_capablity_data();

if ( in_array( $role, $this->roles, true ) ) {
return;
Expand Down Expand Up @@ -575,6 +598,7 @@ public function add_role( $role ) {
* @param string $role Role name.
*/
public function remove_role( $role ) {
$this->load_capablity_data();
if ( ! in_array( $role, $this->roles, true ) ) {
return;
}
Expand Down Expand Up @@ -607,6 +631,7 @@ public function remove_role( $role ) {
* @param string $role Role name.
*/
public function set_role( $role ) {
$this->load_capablity_data();
if ( 1 === count( $this->roles ) && current( $this->roles ) === $role ) {
return;
}
Expand Down Expand Up @@ -708,6 +733,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_capablity_data();
$this->caps[ $cap ] = $grant;
update_user_meta( $this->ID, $this->cap_key, $this->caps );
$this->get_role_caps();
Expand All @@ -722,6 +748,7 @@ public function add_cap( $cap, $grant = true ) {
* @param string $cap Capability name.
*/
public function remove_cap( $cap ) {
$this->load_capablity_data();
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
if ( ! isset( $this->caps[ $cap ] ) ) {
return;
}
Expand All @@ -740,7 +767,8 @@ public function remove_cap( $cap ) {
*/
public function remove_all_caps() {
global $wpdb;
$this->caps = array();
$this->caps = array();
$this->loaded_caps = false;
delete_user_meta( $this->ID, $this->cap_key );
delete_user_meta( $this->ID, $wpdb->get_blog_prefix() . 'user_level' );
$this->get_role_caps();
Expand Down Expand Up @@ -774,6 +802,8 @@ public function remove_all_caps() {
* the given capability for that object.
*/
public function has_cap( $cap, ...$args ) {
$this->load_capablity_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 @@ -874,11 +904,10 @@ 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->cap_key = $wpdb->get_blog_prefix( $this->site_id ) . 'capabilities';
$this->loaded_caps = false;

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

$this->get_role_caps();
Comment on lines -879 to -881
Copy link
Member

Choose a reason for hiding this comment

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

I think removing this without replacing it with some other mechanism to "reset" the flag that capability and role data are loaded will lead to problems. For example, if capabilities and roles were already loaded and then someone calls WP_User::for_site() with another site ID, the WP_User object would still have the old capabilities and roles, not the ones for the site requested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, this appears to be the cause of the test failures for MS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

wp_lazyload_user_meta( array( $this->ID ) );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very familiar with how metadata lazy-loading works, so I feel like I'm not the right person to review this part. For example, one potential caveat here is that user metadata is generally global, but some meta keys only apply to specific sites.

I wonder for instance why this method is called here. Since user metadata is global, why would it need to be loaded (again) when the site is switched? Shouldn't this call be somewhere else, or only be called if it hadn't been called before?

}

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

return $caps;
}

/**
* Load capability data.
*
* @since 6.8.0
*/
private function load_capablity_data() {
if ( $this->loaded_caps ) {
return;
}
$this->caps = $this->get_caps_data();
$this->get_role_caps();
$this->loaded_caps = true;
}
}
2 changes: 1 addition & 1 deletion src/wp-includes/pluggable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ function get_user_by( $field, $value ) {
function cache_users( $user_ids ) {
global $wpdb;

update_meta_cache( 'user', $user_ids );
wp_lazyload_user_meta( $user_ids );

$clean = _get_non_cached_ids( $user_ids, 'users' );

Expand Down
15 changes: 15 additions & 0 deletions src/wp-includes/user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,21 @@ function get_user_meta( $user_id, $key = '', $single = false ) {
return get_metadata( 'user', $user_id, $key, $single );
}

/**
* Queue user meta for lazy-loading.
*
* @since 6.8.0
*
* @param array $user_ids List of user IDs.
*/
function wp_lazyload_user_meta( array $user_ids ) {
if ( empty( $user_ids ) ) {
return;
}
$lazyloader = wp_metadata_lazyloader();
$lazyloader->queue_objects( 'user', $user_ids );
}

/**
* Updates user meta field based on user ID.
*
Expand Down
1 change: 1 addition & 0 deletions tests/phpunit/includes/abstract-testcase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ protected function reset_lazyload_queue() {
$lazyloader->reset_queue( 'term' );
$lazyloader->reset_queue( 'comment' );
$lazyloader->reset_queue( 'blog' );
$lazyloader->reset_queue( 'user' );
}

/**
Expand Down
3 changes: 3 additions & 0 deletions tests/phpunit/tests/post/updatePostAuthorCaches.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ public function test_update_post_author_caches() {
while ( $q->have_posts() ) {
$q->the_post();
}
foreach ( self::$user_ids as $user_id ) {
get_user_meta( $user_id );
}

$args = $action->get_args();
$last_args = end( $args );
Expand Down
5 changes: 2 additions & 3 deletions tests/phpunit/tests/query/cacheResults.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1577,11 +1577,10 @@ public function test_author_cache_warmed_by_the_loop( $fields ) {
$query_1->the_post();
$num_loop_queries = get_num_queries() - $start_loop_queries;
/*
* Two expected queries:
* One expected queries:
* 1: User meta data,
* 2: User data.
*/
$this->assertSame( 2, $num_loop_queries, 'Unexpected number of queries while initializing the loop.' );
$this->assertSame( 1, $num_loop_queries, 'Unexpected number of queries while initializing the loop.' );

$start_author_queries = get_num_queries();
get_user_by( 'ID', self::$author_id );
Expand Down
3 changes: 2 additions & 1 deletion tests/phpunit/tests/user/multisite.php
View file
Open in desktop
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
8 changes: 7 additions & 1 deletion tests/phpunit/tests/user/query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,16 +159,22 @@ public function test_get_all() {
* @ticket 55594
*/
public function test_get_all_primed_users() {
$this->reset_lazyload_queue();
$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