Directory

Check for non-empty array key 1 before using it with `current_user_can()` by Tabrisrp · Pull Request #3140 · WordPress/wordpress-develop · GitHub
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for non-empty array key 1 before using it with current_user_can() #3140

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

Tabrisrp
Copy link

Check for non-empty array key 1 before using it with current_user_can() to avoid PHP warning

Trac ticket: https://core.trac.wordpress.org/ticket/56254


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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiya, thanks for this PR!

Couple of questions:

  1. Are there any tests covering this functionality ? Could a test be added to verify the pre-PHP 8.0 behaviour of the function and to safeguard that the current fix doesn't constitute a BC-break ?
  2. The currently proposed fix will only run the current_user_can() check when $sub_item[1] is non-empty and if the answer is false, it will ignore the submenu item.
    This will protect against the "undefined array index" notice, but I'm not sure it constitutes the correct behaviour.
    As current_user_can() is a security function, I wonder if the condition should be changed to:
    if ( empty( $sub_item[1] ) || ! current_user_can( $sub_item[1] ) )
    That would ignore a $sub_item if no capabilities have been set, as well as when the capabilities don't match those of the current user.

I'm honestly not sure what the correct behaviour should be, but adding a test which would codify the behaviour on PHP < 8.0, would confirm the issue on PHP 8.0+ and would safeguard the fix, should be able to clarify this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants