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 17 commits into
base: trunk
Choose a base branch
from
+96 −12
Open

Lazy load user capabilities in WP_User object #5098

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 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
8de554a
Merge branch 'trunk' into try/lazy-load
spacedmonkey Feb 27, 2025
154a136
Feedback from @peterwilsoncc & @felixarntz
spacedmonkey Feb 27, 2025
2725437
Merge branch 'trunk' into try/lazy-load
spacedmonkey Feb 28, 2025
242545f
Remove lazy loading user meta.
spacedmonkey Feb 28, 2025
b5307f5
Remove unused wp_lazyload_user_meta function
spacedmonkey Feb 28, 2025
18c48c2
Add unit tests for user role and capability handling
spacedmonkey Feb 28, 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();

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

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

/**
* 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();
}

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the key to not loading user meta on every request and is required.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that this is useful, but I don't think it's required for this PR. This line fits perfectly into the scope of https://core.trac.wordpress.org/ticket/63021, but less into lazy-loading the role/cap properties of WP_User.

On another note, if we keep it, could you review my previous question please?

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?

Copy link
Member

Choose a reason for hiding this comment

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

@spacedmonkey Can you please get back to me on this open question?

}

/**
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