wp_list_pluck missing checks

[wpsmith]

If I use wp_list_pluck() to filter an array as a check to see if the array contains any objects with a specific key, errors are thrown.

For example, given an array of objects with keys of slug and name, if I check for ID, wp_list_pluck throws an error.

Here is a gist test: ​https://gist.github.com/wpsmith/91b8cab4d258133b32d3

[wpsmith]

Original Patch

[boonebgorges]

I think this might be a case where isset() checks will paper over developer error. The PHP notices are a sign to the developer that something has gone wrong. Silently skipping items in the array seems like a bad idea to me.

[wpsmith]

Replying to boonebgorges:

I think this might be a case where isset() checks will paper over developer error. The PHP notices are a sign to the developer that something has gone wrong. Silently skipping items in the array seems like a bad idea to me.

I do not believe that (covering developer error) to be the case. The example I am giving is done on purpose. I have a related ticket that I am currently writing up. This ticket and patch is more as a result of #28900.

Please review the ​sample gist again.

[boonebgorges]

Yes, I've looked at the gist. My question is: What is the use case for looping over an array to fetch properties that don't exist on the items of the array? wp_list_pluck() is a utility function, a shortcut. If you're working with inconsistent data, such that you're unsure whether members of the array will have a given property, then you probably ought to be doing your own foreach loop that does any necessary error-checking.

To put the same point another way, your patch assumes that wp_list_pluck() should simply skip items that don't have the specified property. But why should this be expected behavior? Why wouldn't you want array( null, null ) returned?

[wpsmith]

Replying to boonebgorges:

To put the same point another way, your patch assumes that wp_list_pluck() should simply skip items that don't have the specified property. But why should this be expected behavior? Why wouldn't you want array( null, null ) returned?

That is a good point and something that should be considered. As for a good use case, see #34175.

[boonebgorges]

The problems described in #34175 arise from the (incorrect?) use of the get_terms_fields filter and the fields param in get_terms(). Things like update_term_cache() should only be called with full term objects. I don't think that any modifications to wp_list_pluck() can help with that.

Thinking more about the suggestions in this ticket, I think we should not move forward with it. We definitely should not be skipping indexes in the case where they're not set. This will result in the arrays being returned from wp_list_pluck() sometimes having fewer items than the array passed to it. It also means that numerically indexed arrays will have non-consecutive indexes after plucking. Both of these seem bad, and likely to cause errors.

The alternative strategy would be to fill null for indexes that don't have the requested field. So:

$before = array(
    'a' => array( 'foo' => 'bar' ),
    'b' => array(),
    'c' => array( 'foo' => 'baz' ),
);

$after = wp_list_pluck( $before, 'foo' );
// array(
//     'a' => 'bar',
//     'b' => null,
//     'c' => 'baz',
// )

But even this is going to create ambiguity when you have an array where fields can legitimately contain the `null` value.

The potential for confusion is not worth the potential benefits, especially when the benefits are slight - as noted above, PHP notices like the ones you've described can be helpful tips that a developer is doing something wrong.