Lazy load user capabilities in WP_User object #5098
Conversation
spacedmonkey
changed the title
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
The number of expected queries in the cacheResults test has been decreased from two to one. Specifically, the user data query is no longer expected, only the user meta data query remains. The test function has been updated accordingly to reflect this change.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
spacedmonkey
requested review from
peterwilsoncc,
joemcgill,
felixarntz and
mukeshpanchal27
There was a problem hiding this comment.
@spacedmonkey I think there is at least one bug with the code that needs to be addressed (related to switching to another site in Multisite).
Other than that, I think it would be great if you could separate the overall metadata lazyloader integration and handle that in another PR. The role and capability logic itself is expensive enough to be lazy-loaded, which is what most of this PR is about. That in itself is already complex and may have some quirks to consider, so I think separating the two concerns makes the PRs easier to review.
As an aside, I'm not very familiar with metadata lazy-loading, but I am familiar with how WP_User works, so I only feel comfortable with reviewing the latter.
| $this->caps = $this->get_caps_data(); | ||
|
|
||
| $this->get_role_caps(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Indeed, this appears to be the cause of the test failures for MS.
| $this->caps = $this->get_caps_data(); | ||
|
|
||
| $this->get_role_caps(); | ||
| wp_lazyload_user_meta( array( $this->ID ) ); |
There was a problem hiding this comment.
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?
Trac ticket: https://core.trac.wordpress.org/ticket/58001
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.