[wp-trac] [WordPress Trac] #44591: PHP notice if optional argument isn't passed to map_meta_cap()

[WordPress Trac]

#44591: PHP notice if optional argument isn't passed to map_meta_cap()
-------------------------------------------+------------------------------
 Reporter:  henry.wright                   |       Owner:  (none)
     Type:  defect (bug)                   |      Status:  new
 Priority:  normal                         |   Milestone:  Awaiting Review
Component:  General                        |     Version:
 Severity:  normal                         |  Resolution:
 Keywords:  reporter-feedback 2nd-opinion  |     Focuses:
-------------------------------------------+------------------------------
Changes (by mattheweppelsheimer):

 * keywords:  reporter-feedback => reporter-feedback 2nd-opinion


Comment:

 `map_meta_cap()` doesn't consistently check whether `$args` keys are set
 before attempting to access them. Early cases in its `switch` statement
 tend to follow this pattern with `isset()` guards, as in the third line
 from this snippet:

 {{{#!php
 case 'remove_user':
 // In multisite the user must be a super admin to remove themselves.
 if ( isset( $args[0] ) && $user_id == $args[0] && ! is_super_admin(
 $user_id ) ) {
         $caps[] = 'do_not_allow';
 } else {
         $caps[] = 'remove_users';
 }
 break;
 }}}

 Later though, there are several cases where it accesses `$args[0]` without
 an `isset()` first, as in this case which is involved in the `Notice`
 @henry.wright reported (line 136 is the second line in this snippet):

 {{{#!php
 case 'edit_page':
         $post = get_post( $args[0] );
         if ( ! $post ) {
                 $caps[] = 'do_not_allow';
                 break;
         }
 // continuation of `case` omitted...
 }}}

 It is possible that core code never executes any of the `map_meta_cap()`
 cases that assume `$args` are set, without setting them. (I think whether
 or not it does can probably be determined one way or the other with
 certainty, but I haven't looked into that.) If that is so, then `Undefined
 offset` Notices would only come from plugins/themes, and one might argue
 that this is not Core's problem to solve.

 I would argue, however, that this is an area where Core could be made
 friendlier to plugin/theme developers by going ahead and adding those
 guards.

 One might also argue against adding these guards because they might
 obscure underlying issues. So I would propose to use `_doing_it_wrong()`
 and `WP_Error` where appropriate.

 I'm happy to work on a patch for this, but I'd like to get a Core
 Committer's thoughts beforehand, in case my analysis is flawed.

 Is there any good reason **not** to revise `map_meta_cap()` so that it
 always checks if an `$args` index is set before accessing it?

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/44591#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform