Lazy load user capabilities in WP_User object

[spacedmonkey]

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.

[github-actions]

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props spacedmonkey, flixos90, peterwilsoncc.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The 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

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[felixarntz]

@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.

[felixarntz]

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?

[spacedmonkey]

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

[felixarntz]

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?

[felixarntz]

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

[felixarntz]

@spacedmonkey The WP_User changes look almost good to commit, just a few points of minor feedback.

As mentioned, I would like for someone else that is more experienced with WP_Metadata_Lazyloader to review that portion of the PR.

[felixarntz]

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

[spacedmonkey]

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.

[felixarntz]

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.

[spacedmonkey]

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?

[felixarntz]

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

[felixarntz]

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.

[peterwilsoncc]

I've added a couple of notes and a question inline.

The change to cache_users, does make the change so user meta data is always lazy loaded and there doesn't appear to be a way for developers to trigger the active loading of user meta.

Based on manual testing it appears pretty good.

[felixarntz]

@spacedmonkey @peterwilsoncc 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?

[spacedmonkey]

@peterwilsoncc @felixarntz @joemcgill As requested, I have removed all the lazy loading user meta stuff. This makes this patch much much smaller and easily to review.

I have added some tests to valid that get_role_caps continues to work. Other existing tests cover add_cap, remove_cap, remove_all_caps, has_cap and set_role still work.

[spacedmonkey]

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?

[spacedmonkey]

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.