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.