Lazy load user capabilities in WP_User object

[spacedmonkey]

WP_User object are used throughout WordPress and are loaded on the front end ​in WP_Query. This means creating a WP_User object and this calls for_site method. This calls to user meta and setups capability data. For a page with multiple users on it, this setup of users, can be done multiple times. Loading user meta for all the users, can be a problem, as this can be a lot of rows of data. For example, this can result in 150 rows being loaded from the database for just 10 users. For multisites or sites that have plugins that are heavy users of user meta, that number can get much higher.

User meta is only loaded along with users, as it is needed for capability data. To improve performance, find someway to lazy load the capability only when they are used and also lazily load user meta at the same time.

Trac ticket: https://core.trac.wordpress.org/ticket/58001

[prettyboymp]

#59430 was marked as a duplicate.

[prettyboymp]

Similar ticket in #59430 - improve the loading performance of usermeta on network sites. Though the performance issue is somewhat different, it would be great if the solution we found solved both problems.

Core functionality like user roles and capabilities will likely need some extra special handling like what ​PR#5098 does. However, I still believe we need an API for:

• Segmenting usermeta by blog to avoid autoloading user meta from other blogs on the network.
• Allowing some usermeta to designated as non-autoload for data the requires rare usage and/or takes up too much memory to warrant loading/caching with the rest of the usermeta.

[flixos90]

Per ​Slack discussion, it's too late now to get this in, because it's a potentially risky change and hasn't been reviewed yet. To be clear, I definitely think we should land this, but safer to do it early in the 6.8 cycle.

[spacedmonkey]

I rebased the PR for you @flixos90. Let me know if you want to disucss this ticket.

@mukesh27 do you mind taking a look at the PR?

[flixos90]

I'm going to take a look at the PR here soon.

[spacedmonkey]

@flixos90 @joemcgill The core performance problem I want to solve here is this:

In WP_Query, the update_post_author_caches function is called in the_post method. This is just to prime all user objects in memory, as this user object is used in generate_postdata, that calls get_userdata. This sets a global authordata that should be a WP_User object.

On a simple site with one user, that is not really a problem. The single user is loaded with its user meta. But once you have a set of authors, from all different users, like 10 posts with 10 authors, then 10 lots of user meta is loaded. If you have plugins that add lots of user meta keys, like Woo does, then, if you are loading 10 users, with say 25 pieces of user meta, that is already, 250 rows of meta loading. The real problem is if then this is done on a multisite. If you have 10 users, with 25 pieces of meta ( per site ) and they are across 15 sites, that is 10 x 25 x 15, that is 3750 rows. Even with object caching enabled, that is a lot of data that is loaded.

As far as I can see, not many plugins or themes use this global authordata. We could consider loading setting the authordata global, but creating a WP_User object without loading user meta. If you wanted to use the authordata global, it would still be a valid WP_User, but would not need the user meta. I would love to be able to remove the global altogether, but that would be a BC break.

The current solution I have put forward is clever and fixes the problem above but the magic methods are a bit of a workaround and have some risk attached.

Thoughts.

[peterwilsoncc]

@spacedmonkey Are you able to fix the issue highlighted by the failing tests for Multisite after a site switch.

[spacedmonkey]

@peterwilsoncc before I spent anymore time on this ticket, I would like some alignment that the magic method is the right approach here. Once we are aligned, we can work on Multisite / fixing tests. Without the support of the core committers, I do not want to invest in this ticket.

[flixos90]

@spacedmonkey @peterwilsoncc The approach of the PR using magic methods looks good to me. There is some related outstanding feedback on the PR, but the overall approach should work.

My other feedback was about potentially breaking up the PR in two parts, because right now it does two things:

• Alter the WP_User class to lazy-load capability data instead of when the class is instantiated.
• Add support for user metadata to WP_Metadata_Lazyloader.

These things are related, but technically separate. If y'all feel good about keeping both in the one PR, that works for me. In that case though, I would prefer for someone other than me (@peterwilsoncc?) to sign off on the WP_Metadata_Lazyloader related changes. I feel confident enough in how WP_User works to review that part of the PR, but not the WP_Metadata_Lazyloader part.

[joemcgill]

@spacedmonkey I'm reviewing early tickets for 6.8 and we're planning to either commit or punt those tickets this week ahead of Beta 1. Do you think you'll have time to address the feedback on this one or should we move it to the 6.9 milestone?

[spacedmonkey]

@joemcgill i am happy with the code as is. I need to add some unit tests to test the number of databases are being saved. I will try to add them this weekend. If I add them and get another review, I think this can land in 6.8.

I am realizing just now that the change of making the $caps, $roles, and $allcaps properties protected may be considered a breaking change, albeit only relevant for edge cases.

They're still readable via magic getter and isset-er, but in theory a plugin doing some very niche capability modification stuff may be _writing_ these properties too. With the current state of this PR, that wouldn't work anymore.

I'm not overly concerned about this, since doing so would be a bad practice, but I still wanted to flag it. I think we have two options here:

• Either accept this as a edge-case BC break we're willing to make, punt this to 6.9 to land early in the milestone, and cover it in a dev note.
• Or add conditions for these to the __set() method to maintain backward compatibility (which is definitely more forgiving, but may continue encouraging this problematic pattern).

WDYT?

If you are change the properties manaully, I feel like that are a pretty dangerous way of changing a user role. There are other filters and ways of doing this that are less problematic. I do believe this needs a detail dev note, but I feel like we should not enable this edge.

[flixos90]

Based on the latest discussion on the PR ​https://github.com/WordPress/wordpress-develop/pull/5098, I don't think this is ready to land this late in the cycle.

I'm on board with removing public write access to these properties, but because this is technically a breaking change (albeit only for edge-cases that are strongly discouraged), we should land it early next cycle. FWIW, the ticket is already marked early, and it's definitely too late for that in 6.8.

@spacedmonkey FYI there are some merge conflicts plus some open feedback above from Felix (whom I accidentally requested another review from, sorry!)

[sachinrajcp123]

Add lazy loading of user capabilities in WP_User to defer loading role caps and associated user meta until actually needed, improving performance for pages with many users.

[spacedmonkey]

All feedback has been actioned and PR has been rebased.

@westonruter @mukeshpanchal27 Feedback actioned, can you take a look.

[spacedmonkey]

In 60915:

Users: Lazy load user capabilities in WP_User object.

Convert the WP_User object properties caps, roles, and allcaps to protected, and introduce lazy loading for capabilities. These properties are now populated only when first accessed.

The existing magic methods (get, set, and unset) have been updated to maintain backward compatibility, ensuring that reading or modifying these formerly public properties continues to work as expected.

Ensure that these properties are initialised when calling remove_all_caps(), remove_cap(), has_cap(), add_role(), and set_role() methods.

Props spacedmonkey, flixos90, peterwilsoncc, mukesh27, westonruter, swissspidy, prettyboymp.
Fixes #58001.

[TimothyBlynJacobs]

Making those properties protected broke our tests. get_object_vars (and similar functions) behave differently now that those properties are protected. In get_object_vars they are completely stripped.

This also broke WP_User::to_array(), that's a straightforward fix which is nice.

But considering how caps and roles are intrinsically security related, this change seems particularly risky.

Reopening because I think we must fix the WP_User::to_array() behavior.

I'm less confident that the broader get_object_vars like issues are a blocker. But it seems most of the discussion focused on folks writing to those properties.

[bradshawtm]

Like the comment above, I ran into issues with get_object_vars() not populating those properties; it's easy enough to work around, but I'd worry about code unaware of these changes that makes assumptions based on a blind use of get_object_vars().

I also ran into a case in our code where we were modifying keys in the allcaps array, though that type of behavior is probably specifically what the change is trying to gate, so no concerns on that point.

[spacedmonkey]

Reopening because I think we must fix the WP_User::to_array() behavior.

@TimothyBlynJacobs I am not sure what you mean here. WP_User::to_array() looks like this

public function to_array() {
   return get_object_vars( $this->data );
}

This is what that method returns before and after
Before

array(12) {
  ["ID"]=>
  string(1) "2"
  ["user_login"]=>
  string(10) "themedemos"
  ["user_pass"]=>
  string(34) "$P$BfSW/JC68W63cqm4GB5iv2YXDDGGAY1"
  ["user_nicename"]=>
  string(10) "themedemos"
  ["user_email"]=>
  string(29) "themeshaperwp+demos@gmail.com"
  ["user_url"]=>
  string(0) ""
  ["user_registered"]=>
  string(19) "2024-11-12 22:19:43"
  ["user_activation_key"]=>
  string(0) ""
  ["user_status"]=>
  string(1) "0"
  ["display_name"]=>
  string(12) "Theme Buster"
  ["spam"]=>
  string(1) "0"
  ["deleted"]=>
  string(1) "0"
}

After

array(12) {
  ["ID"]=>
  string(1) "2"
  ["user_login"]=>
  string(10) "themedemos"
  ["user_pass"]=>
  string(34) "$P$BfSW/JC68W63cqm4GB5iv2YXDDGGAY1"
  ["user_nicename"]=>
  string(10) "themedemos"
  ["user_email"]=>
  string(29) "themeshaperwp+demos@gmail.com"
  ["user_url"]=>
  string(0) ""
  ["user_registered"]=>
  string(19) "2024-11-12 22:19:43"
  ["user_activation_key"]=>
  string(0) ""
  ["user_status"]=>
  string(1) "0"
  ["display_name"]=>
  string(12) "Theme Buster"
  ["spam"]=>
  string(1) "0"
  ["deleted"]=>
  string(1) "0"
}

I don't see caps, roles, and allcaps keys in either array, so I don't see the breakable here.

I also ran into a case in our code where we were modifying keys in the allcaps array, though that type of behavior is probably specifically what the change is trying to gate, so no concerns on that point.

I used a magic setter, so this should work. @bradshawtm you mind testing?

As for get_object_vars, this was considered, but it feels super edge case, that you would use that function and get those properties, I maybe wrong.

Adding needs dev note, as this change might effect third party code.

[TimothyBlynJacobs]

I am not sure what you mean here. WP_User::to_array() looks like this

Yep, my mistake. Apologies for the confusion.

that you would use that function and get those properties

I think it's less folks doing specifically get_object_vars( $user )['allcaps'] all in one go. But more the User object being moved around or serialized into a plain object.

[bradshawtm]

As for get_object_vars, this was considered, but it feels super edge case

I'm not convinced this point is edge case given it came up twice shortly after this change was merged. I suspect directly serializing a WP_User object is fairly common practice. I just realized this also affects json_encode()/wp_json_encode(), neither of which will export the protected properties even if populated:

function test_json_encode() {
        $user = new WP_User(1);
        error_log(var_export( wp_json_encode( $user ), true)); // no roles
        $user->roles;
        error_log(var_export( wp_json_encode( $user ), true)); // no roles
}
add_action('wp_footer', 'test_json_encode');

---

I used a magic setter, so this should work.

One can't directly modify the array elements of a protected property, and I don't think there's a way for the magic setter to work around that.

This is discussed as being a breaking change above, and I'm personally okay with a dev note on this point rather than allowing the edge case. That said, here's a simple example:

function test_changed_behavior() {
        $user = new WP_User(1);
        error_log(var_export($user, true)); // `roles` shows as array()
        error_log(var_export($user->roles, true)); // `roles` shows as array( 'administrator' )

        // these worked before the change, but not after. Each gives a warning: `Indirect modification of overloaded property`
        array_shift( $user->roles );
        $user->roles[] = 'subscriber';
        $user->roles[0] = 'editor';

        error_log(var_export($user->roles, true)); // `roles` shows as array( 'administrator' )
}
add_action('wp_footer', 'test_changed_behavior');

function test_mitigation() {
        $user = new WP_User(1);
        error_log(var_export($user, true)); // `roles` shows as array()
        error_log(var_export($user->roles, true)); // `roles` shows as array( 'administrator' )

        // Workaround: modify a different var
        $roles = $user->roles;
        array_shift( $roles );
        $roles[] = 'subscriber';
        $roles[0] = 'editor';
        $user->roles = $roles;

        error_log(var_export($user->roles, true)); // `roles` shows as array( 'administrator' )
}
add_action('wp_footer', 'test_mitigation');

[peterwilsoncc]

I'm strongly inclined to revert this change.

I don't think a dev-note is adequate for back-compat changes to roles and caps as a third party developer may be relying on the legacy data to block access to private data.

[spacedmonkey]

We already have other examples of where properties on this class have be deprecated like in [18504].

I see no point on adding to caps, allcaps and roles properties on a class instance. These changes would not presist, to do that, you need to call add_cap and add_role. To get this data, there are methods like get_role_caps.

If we revert this change, it will mean we have to make this as wontfix and it also means that #63021 will not be possible.

Searching the plugin repo, there do seem to some examples, but not all them seem to related to the WP_User object.

​https://www.wpdirectory.net/search/01K7MH8A28VGSHHV41PVG891JE
​https://www.wpdirectory.net/search/01K7MHBCP874AFAMPQXZ5T68VE
​https://www.wpdirectory.net/search/01K7MHCEJ4CEF3WQNV52CGSAD8

[bradshawtm]

The bigger concern is the use of get_object_vars(), json_encode(), and wp_json_encode().

Those are a bit more difficult to search, but under the assumption of $user* representing WP_User, this turns up several security/login/backup plugins:

• ​https://www.wpdirectory.net/search/01K7MJWY97ZXVCK1X9XC10MX9P
• ​https://www.wpdirectory.net/search/01K7MK8GQZRT6H0E6Q8XC0R1NQ

[spacedmonkey]

I have looked into this.

We could use the JsonSerializable interface and the jsonSerialize method, to make sure that json_encode continues to work.

   public function jsonSerialize() {
        $this->load_capability_data();
        return array(
            'data'    => $this->data,
            'ID'      => $this->ID,
            'caps'    => $this->caps,
            'cap_key' => $this->cap_key,
            'roles'   => $this->roles,
            'allcaps' => $this->allcaps,
            'filter'  => $this->filter,
            'site_id' => $this->site_id,
        );
    }

We can also add __serialize method, to handle the serialize use case in PHP 7.4+.

Still working on a fix for get_object_vars.

Replace get_userdata() with get_authordata() for optimized user data retrieval and introduce short initialization in WP_User.

Trac ticket: https://core.trac.wordpress.org/ticket/58001

• Made caps, roles, and allcaps public to improve access flexibility.
• Introduced $short_init to enable optimized initialization for use cases where full capability and role loading is unnecessary.
• Removed redundant lazy-loading logic for these properties, simplifying the class structure.
• Adjusted documentation for affected properties and methods.

Trac ticket: https://core.trac.wordpress.org/ticket/58001

[spacedmonkey]

I put together ​PR that makes the caps, roles and allcaps properties again. The biggest issue this fixes, is loading user meta on the frontend. This new change, add a short_init option, that loads the user object, but skips loading roles, cap and allcaps. This is an opt-in feature and opt-in in a number of places in core. This stop user meta loading on the front end.

The other downside, is the if you use the $authordata global, the properties will not be setup. This feel much more edge the current solution. But I will check the plugin repo.

[spacedmonkey]

@peterwilsoncc @westonruter @bradshawtm @TimothyBlynJacobs Any chance of a review on my ​PR with beta around the corner.

@jonnynews There's a couple of failing unit tests, are you able to look at those.

To ensure the caps are available when using get_object_vars(), are you able to add this to tests/phpunit/tests/user/capabilities.php. The data provider already exists.

/**
 * Ensure get_object_vars includes allcaps.
 *
 * @ticket 58001
 *
 * @dataProvider data_single_site_roles_to_check
 */
public function test_get_object_vars_includes_roles( $role ) {
        $user_id        = self::$users[ $role ]->ID;
        $user           = new WP_User( $user_id );
        $vars           = get_object_vars( $user );
        $primitive_caps = $this->getPrimitiveCapsAndRoles();

        $expected = array();
        foreach ( $primitive_caps as $cap => $roles ) {
                if ( in_array( $role, $roles, true ) ) {
                        $expected[] = $cap;
                }
        }

        $this->assertArrayHasKey( 'allcaps', $vars, 'WP_User object vars should include allcaps key.' );

        $actual = $vars['allcaps'];
        $actual = array_keys( $actual );

        // Remove special cases.
        $special_cases = array(
                // Role names.
                'administrator',
                'editor',
                'author',
                'subscriber',
                'contributor',

                // Granted via `user_has_cap`.
                'resume_plugins',
                'resume_themes',
                'view_site_health_checks',

                // Other capabilities.
                'manage_links',
                'unfiltered_upload',
        );

        $actual   = array_diff( $actual, $special_cases );
        $expected = array_diff( $expected, $special_cases );
        $this->assertSameSets( $expected, $actual, "User with the {$role} role should have the correct primitive capabilities in allcaps." );
}

[jorbin]

With the freeze for 6.9 beta 1 in less than an hour and failing unit tests on the PR, I'm punting this to 7.9

[TimothyBlynJacobs]

@jorbin I don't think this can be punted currently. Work was merged to trunk already. So we need to determine if we are going to fully revert this, or commit @spacedmonkey's latest patch.

[jorbin]

In 61037:

Users: Revert Lazy load user capabilities in WP_User object.

The change from public to protected broke tests for extenders and this also broke WP_User::to_array().

Reverts [60915].

Props davidbaumwald, ellatrix, timothyblynjacobs, welcher, spacedmonkey, bradshawtm, peterwilsoncc, jorbin.
See #58001.

[jorbin]

Thanks @TimothyBlynJacobs. I've reverted it for now but if the more complete version is something that should be considered to go in before beta 2, I think that discussion should take place but moving to 7.0 for the time being.

[spacedmonkey]

I am away from keyboard at the moment. Thanks for the revert. I didn’t have time to do it myself. Sadly, this ticket is heavily linked to another one #63021, as this was also committed, core tests will fail.

Either that ticket needs to be reverted or tests updated.
Lazy loading capabilities goes hand in hand with lazy loading user meta. Capabilities are stored in user meta, if we don’t load them lazily, then user meta need to eagerly loaded.

Cc @jorbin @TimothyBlynJacobs

[davidbaumwald]

In 61038:

Users: Revert Lazy-load user meta.

With [60915] reverted, this changeset is also being reverted to resolve test failures due to common code.

Reverts [60989].

Follow-up to [61037].

Props jorbin, ellatrix, spacedmonkey.
See #63021, #58001.