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

Do strict type checking when looking up selected notification roles #1

Closed
wants to merge 2 commits into from

Conversation

edwardcasbon
Copy link

Because the $roles array is an array of boolean values and there's no strict type checking the in_array() will always return true, and if the key doesn't exist in $roles throws a warning.

This PR improves that by adding strict type checking, and also using array_key_exists to check for existance of the key.

@joho1968
Copy link
Owner

joho1968 commented Sep 2, 2024

Thank you! But I'm not sure I follow...

An example of $roles would be this:

array (
  'install_plugins' => true,
  'update_themes' => true,
  'install_themes' => true,
  'update_core' => true,
  'list_users' => true,
  'remove_users' => true,
  'promote_users' => true,
  'edit_theme_options' => true,
  'delete_themes' => true,
  'export' => true,
  'administrator' => true,
)

An example of $notify_roles would be this:

array (
  0 => 'administrator',
  1 => 'editor',
  2 => 'author',
  3 => 'contributor',
  4 => 'subscriber',
)

In the foreach() construct, it goes through the $notify_roles array, extracting each individual role as "administrator", "editor", "author", "contributor", and "subscriber" in this case. It does this to the variable $role.

It then checks if $role, for example, "administrator" is in the array $roles. And if it is, and only then, does it check if the $role key in $roles is true. So since booleans are evaluated short-circuit, it should never even get to the second condition, if the first, in_array(), returns false.

If anything, the in_array() call should probably be array_key_exists() instead.

But maybe I'm way off here and you mean something different? 🤔

@joho1968 joho1968 added the question Further information is requested label Sep 2, 2024
@joho1968
Copy link
Owner

joho1968 commented Sep 2, 2024

So I'd say this should work:

        foreach( $notify_roles as $role ) {
            if ( array_key_exists( $role, $roles ) && $roles[$role] ) {
                return( true );
            }
        }

@joho1968
Copy link
Owner

joho1968 commented Sep 2, 2024

Implemented for next version.

@joho1968 joho1968 closed this Sep 2, 2024
@edwardcasbon edwardcasbon deleted the patch-1 branch September 2, 2024 13:55
@joho1968 joho1968 self-assigned this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants