Opened 2 years ago

Last modified 2 years ago

#16895 new enhancement

wp_list_pluck cannot be used with arrays of references

Reported by: dd32 Owned by:
Priority: normal Milestone: Future Release
Component: General Version: 3.1
Severity: normal Keywords: has-patch
Cc: scribu

Description

Example code:

$one = array('num' => 1);
$two = array('num' => 2);
$input = array( &$one, &$two );

$nums = wp_list_pluck($input, 'num');

var_dump($nums, $input);

Example output:

array
  0 => &int 1
  1 => &int 2
array
  0 => &int 1
  1 => &int 2

Expected output:

array
  0 => int 1
  1 => int 2
array
  0 => &
    array
      'num' => int 1
  1 => &
    array
      'num' => int 2

This is caused by wp_list_pluck using the same variable name for the input array, and then overwriting it for the return.

One such real life example, is $wp_query->comments

See patch

Attachments (6)

16895.diff (534 bytes) - added by dd32 2 years ago.
16895.patch (454 bytes) - added by hakre 2 years ago.
Remove the cause and the sympton.
test-pluck.php (1.6 KB) - added by hakre 2 years ago.
This is how I tested changes
16895.2.patch (483 bytes) - added by hakre 2 years ago.
remove possible references
16895.3.patch (757 bytes) - added by hakre 2 years ago.
allow references in fields
16895.4.patch (788 bytes) - added by hakre 2 years ago.
allow references in fields

Download all attachments as: .zip

Change History (18)

dd322 years ago

  • Cc scribu added
  • Keywords reporter-feedback added
  • Type changed from defect (bug) to enhancement

The function states that it the $list parameter is an array that contains arrays or objects:

/**
 * Pluck a certain field out of each object in a list
 *
 * @since 3.1.0
 *
 * @param array $list A list of objects or arrays
 * @param int|string $field A field from the object to place instead of the entire object
 * @return array
 */
function wp_list_pluck( $list, $field ) {

It does not state that it operates on an array that contains referecens to other arrays.

As well as it doesn't state that it operates on an array that contains references to other objects.

That's not what it has been made fore. So why do you expect it to be working with references?

And if it is expected to be working with references, shouldn't it return references then?

To demonstrate an overwrite, see this example:

$one = array('num' => 1);
$input = array( &$one, &$one );

$nums = wp_list_pluck($input, 'num');

var_dump($nums, $input);

will do notices on trunk and set both keys to NULL in $nums.

But I still can not see how that should apply to $wp_query->comments (I didn't get it filled in my test script either, just asking).

  • Keywords reporter-feedback removed

hakre: Instead of academic debates over unlikely scenarios, would you agree that the proposed enhancement is useful and doesn't actually break anything else?

hakre2 years ago

Remove the cause and the sympton.

Replying to scribu:

hakre: Instead of academic debates over unlikely scenarios, would you agree that
the proposed enhancement is useful and doesn't actually break anything else?

Well I thought the function is already doing the job it has been made for - see [15686], #15016. :)

However, you find a new patch attached that will de-reference array members before setting them again. This should make the original cause of the problem more visible.

hakre2 years ago

This is how I tested changes

Ok, so you found a way to make it work even better. Great! :)

Well I thought the function is already doing the job it has been made for - see [15686], #15016. :)

The truth is I never even thought about references when I wrote it.

comment:7 follow-up: ↓ 8   scribu2 years ago

By the way, you should add a short explanatory comment to the line with unset(), as it's not obvious why it's there.

comment:8 in reply to: ↑ 7   hakre2 years ago

Replying to scribu:

By the way, you should add a short explanatory comment to the line with unset(), as it's not obvious why it's there.

Do you really think it is necessary to explain how PHP works? What should be left there? A link to the PHP manual? What's the part you think needs to be explained in a comment? That unset is removing references from variables?

No, I wasnt suggesting explaining what unset() does, but the reason it's used there:

unset( $list[ $key ] ); // avoids problems with reference values

hakre2 years ago

remove possible references

But I still can not see how that should apply to $wp_query->comments (I didn't get it filled in my test script either, just asking).

If you tested on a page which might possibly have it set, you'd find it's an array of references to arrays.

For readability, I'd prefer to have the output list separate from the input list, rather than just juggling PHP reference's.

The other thing which this will protect against is a reference to an array being passed in to $list, which while deprecated in PHP (Call-time pass-by-reference) some developers may still do.

hakre2 years ago

allow references in fields

Replying to dd32:

For readability, I'd prefer to have the output list separate from the input
list, rather than just juggling PHP reference's.

Well it's actually not juggling and normally foreach opereates on a copy of the array ...

The other thing which this will protect against is a reference to an array being
passed in to $list, which while deprecated in PHP (Call-time pass-by-reference)
some developers may still do.

... but not in that case, right.

As references are so popular, one could argue that the function should properly take over referenced values in the fields as well.

Version 1, edited 2 years ago by hakre (previous) (next) (diff)

And again a little explanation that the & is used to deal with possible references in fields.

hakre2 years ago

allow references in fields

Note: See TracTickets for help on using tickets.