When filling user names, be sure to pull from entire network - #194
When filling user names, be sure to pull from entire network#194boonebgorges wants to merge 1 commit into
Conversation
On Multisite installations, the user_ids in H5P results may correspond to network users who are not members of the current site. The proposed change tells WP to pull data corresponding to these user_ids, regardless of whether they have a role on the current site.
otacke
left a comment
otacke
left a comment
There was a problem hiding this comment.
Hi! I was made a community maintainer for H5P Group's repositories a few weeks ago, and I am still in the process trying to tackle all the open issues and pull requests.
I am terribly sorry that you have not received a reply before.
Thanks for your pull-request in the name of H5P Group. I can confirm the underlying issue and I can confirm that your fix also works.
I think the change in https://github.com/h5p/h5p-wordpress-plugin/pull/194/changes#diff-e6e2c121f268a45ddf301878b952b053dd2f3ae228c9ebac7c046d6219b75efaR329 is not strictly necessary - it is used for showing the list of H5P contents, and you must have a role in a blog to be able to create content. So, even without setting blog_id to null, the results will contain the user name, right? So, I'd request to revert this. I'd not know why to query the global user table instead, but I am willing to admit ignorance :-)
Just mentioning: There's one more call to get_users in class-h5p-privacy-policy.php, but that one is meant to address the blog that it is called from, so there's no need to widen the scope by setting blog_id to null.
| */ | ||
| $wp_users = get_users( | ||
| array( | ||
| 'blog_id' => null, |
There was a problem hiding this comment.
I don't think this is necessary, is it?
Here get_users is meant to fetch the data of users who created content on this blog. And do so, the user must have a role on that site if I am not mistaken. Having 'blog_id' => null here won't break anything, but I'd not know why all global users should be queried instead.
There was a problem hiding this comment.
Good question. It's possible for a H5P content to belong to a user without a role on the blog. The two main cases I can think of are: (a) The user used to have a role on a blog, to create H5P content, but then later to left the blog; and (b) a super-admin created H5P content without having a blog role. In either case, you'd still want the attribution to work properly. For this reason, I'd recommend leaving the blog_id argument here. That being said, if you think it's unnecessary, it can be removed.
There was a problem hiding this comment.
@boonebgorges Convinced! Does not hurt to leave it in.
github-project-automation
Bot
moved this from Backlog
to Waiting for Contributor
in H5P Open Source Project
otacke
moved this from Waiting for Contributor
to Verified by Community Maintainer
in H5P Open Source Project
2 participants
When filling user names using
get_users(), the default WP behavior is to pull users who have a role on the current site. If H5P is running on a specific site in a Multisite network, and if a logged-in user interacts with a piece of H5P content but that user does not have a role on the specific site where H5P is active, then the user's name will be left blank in the Dashboard 'Results' views. The proposed change tells WP to pull up the user corresponding to the provideduser_id, regardless of whether that user has a role on the current site.