Closed vaurdan closed 1 month ago
[!IMPORTANT]
Review skipped
Auto reviews are disabled on base/target branches other than the default branch.
Base branches to auto review (4)
* develop * add/.* * fix/.* * update/.*Please check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository. To trigger a single review, invoke the@coderabbitai review
command.You can disable this status message by setting the
reviews.review_status
tofalse
in the CodeRabbit configuration file.
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Also, shouldn't it be checking all of the roles and not just the presumed first one? Or probably better yet, just using current_user_can()
?
I've held off on rolling out the hotfix until @vaurdan or @acicovic are online.
Hey @rinatkhaziev and @WPprodigy, thank you for chiming in and for your suggestions.
I don't remember why I ended up using $current_user->roles[0]
, and we can consider testing the alternatives proposed. Since the current fix should work though, we're happy to merge it as-is and release a 3.16.1 today/tomorrow.
We can then see if any of the proposed solutions are better and act accordingly.
Let me know of your thoughts.
In some cases, this code could be called before the user object would be initialized properly. Haven't tested, but I don't know if current_user_can()
would work here.
@rinatkhaziev @WPprodigy thank you both for your feedback!
Since it's actually possible to have multiple roles, I did some changes to the permissions checking to handle that scenario. A good side effect, is that it should also handle when there are users with invalid roles, which seems to be the case of the warning.
I have tested these changes locally, with a user that had two roles, and it is working as expected. @acicovic what do you think of this solution?
This looks good to me provided we want to have a permissive approach (the role with the most privileges wins).
I'm wondering though if this could raise edge cases (some customers wanting a restrictive approach instead), which would also warrant some UI work. We could provide this in the future though, if we stumble upon it.
Thoughts?
I'm wondering though if this could raise edge cases (some customers wanting a restrictive approach instead), which would also warrant some UI work. We could provide this in the future though, if we stumble upon it.
That's a fair point. However, having multiple roles on the same user is not something that is supported out of the box, it requires either a plugin or custom code. I think that, in that scenario, we could offer them a filter, and they can tweak it to their needs.
Agreed that this is an edge case. Hopefully we may never need to deal with it.
Description
There are certain scenarios where the user role might be invalid - the role has been deleted, for example - and therefore, the roles array in the
WP_User
object might not have any data on the0
index.This PR adds an additional validation that checks if this array key is set, and if not, returns false.
This prevents an issue where a
Warning: Undefined array key 0
warning can be thrown if there is no valid role associated with the user.Motivation and context
Improve the reliability of the plugin's codebase and prevent PHP errors and warnings from filling the logs.
How has this been tested?
Manually tested.
Summary by CodeRabbit