wp_list_pluck cannot be used with arrays of references

[dd32]

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

[hakre]

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?

[hakre]

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).

[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?

[hakre]

Remove the cause and the sympton.

[hakre]

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.

[hakre]

This is how I tested changes

[scribu]

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.

[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.

[hakre]

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?

[scribu]

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

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

[hakre]

remove possible references

[dd32]

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.

[hakre]

allow references in fields

[hakre]

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.

At least it's sort of re-using an input variable for output which can be considered ugly, yes.

But it's actually not juggling with references as normally foreach operates 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.

[hakre]

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

[hakre]

allow references in fields

[chriscct7]

Patch needs a refresh. Some parts (using non-loop list variable) was implemented in #28666

[dd32]

Having just run into this again.. Lets see if we can't finally fix this..

[dd32]

Tests: ​https://travis-ci.org/dd32/wordpress-develop/builds/330198311

[dd32]

In 42526:

Tests: Remove redundant assertEquals() calls in listFilter tests.

As these tests are already validating that two arrays match, checking that the counts also match before is redundant and makes it harder to debug when the test fails.

See #16895

[dd32]

In 42527:

General: Allow wp_list_pluck() to operate on arrays of references without overwriting the referenced items.

Fixes #16895.