WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34172 closed defect (bug) (wontfix)

wp_list_pluck missing checks

Reported by: wpsmith Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: General Keywords:
Focuses: performance Cc:

Description

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

Attachments (1)

34172.functions.php.patch (1.4 KB) - added by wpsmith 4 years ago.
Original Patch

Download all attachments as: .zip

Change History (6)

@wpsmith
4 years ago

Original Patch

#1 follow-up: @boonebgorges
4 years ago

  • Keywords close added

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.

#2 in reply to: ↑ 1 @wpsmith
4 years ago

  • Keywords close removed

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.

#3 follow-up: @boonebgorges
4 years ago

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?

#4 in reply to: ↑ 3 @wpsmith
4 years ago

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.

#5 @boonebgorges
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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.
Note: See TracTickets for help on using tickets.