WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#16895 new enhancement

wp_list_pluck cannot be used with arrays of references

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

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 3 years ago.
16895.patch (454 bytes) - added by hakre 3 years ago.
Remove the cause and the sympton.
test-pluck.php (1.6 KB) - added by hakre 3 years ago.
This is how I tested changes
16895.2.patch (483 bytes) - added by hakre 3 years ago.
remove possible references
16895.3.patch (757 bytes) - added by hakre 3 years ago.
allow references in fields
16895.4.patch (788 bytes) - added by hakre 3 years ago.
allow references in fields

Download all attachments as: .zip

Change History (18)

dd323 years ago

comment:1 scribu3 years ago

  • Cc scribu added

comment:2 hakre3 years ago

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

comment:3 hakre3 years ago

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

comment:4 scribu3 years ago

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

hakre3 years ago

Remove the cause and the sympton.

comment:5 hakre3 years ago

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.

hakre3 years ago

This is how I tested changes

comment:6 scribu3 years ago

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: scribu3 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 hakre3 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?

comment:9 scribu3 years ago

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

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

hakre3 years ago

remove possible references

comment:10 dd323 years ago

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.

hakre3 years ago

allow references in fields

comment:11 hakre3 years ago

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.

Last edited 3 years ago by hakre (previous) (diff)

comment:12 hakre3 years ago

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

hakre3 years ago

allow references in fields

Note: See TracTickets for help on using tickets.