Skip to content

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

Closed
edwardcasbon wants to merge 2 commits into
joho1968:masterfrom
edwardcasbon:patch-1
Closed

Do strict type checking when looking up selected notification roles#1
edwardcasbon wants to merge 2 commits into
joho1968:masterfrom
edwardcasbon:patch-1

Conversation

@edwardcasbon

Copy link
Copy Markdown

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

joho1968 commented Sep 2, 2024

Copy link
Copy Markdown
Owner

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

joho1968 commented Sep 2, 2024

Copy link
Copy Markdown
Owner

So I'd say this should work:

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

@joho1968

joho1968 commented Sep 2, 2024

Copy link
Copy Markdown
Owner

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