Make WordPress Core

Opened 13 years ago

Closed 6 years ago

Last modified 5 years ago

#16895 closed enhancement (fixed)

wp_list_pluck cannot be used with arrays of references

Reported by: dd32's profile dd32 Owned by: dd32's profile dd32
Milestone: 5.1 Priority: normal
Severity: normal Version: 3.1
Component: General Keywords: needs-refresh dev-feedback
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 (7)

16895.diff (534 bytes) - added by dd32 13 years ago.
16895.patch (454 bytes) - added by hakre 13 years ago.
Remove the cause and the sympton.
test-pluck.php (1.6 KB) - added by hakre 13 years ago.
This is how I tested changes
16895.2.patch (483 bytes) - added by hakre 13 years ago.
remove possible references
16895.3.patch (757 bytes) - added by hakre 13 years ago.
allow references in fields
16895.4.patch (788 bytes) - added by hakre 13 years ago.
allow references in fields
16895.2.diff (4.4 KB) - added by dd32 6 years ago.
Tests: https://travis-ci.org/dd32/wordpress-develop/builds/330198311

Download all attachments as: .zip

Change History (27)

@dd32
13 years ago

#1 @scribu
13 years ago

  • Cc scribu added

#2 @hakre
13 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?

#3 @hakre
13 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).

#4 @scribu
13 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?

@hakre
13 years ago

Remove the cause and the sympton.

#5 @hakre
13 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.

@hakre
13 years ago

This is how I tested changes

#6 @scribu
13 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.

#7 follow-up: @scribu
13 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.

#8 in reply to: ↑ 7 @hakre
13 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?

#9 @scribu
13 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

@hakre
13 years ago

remove possible references

#10 @dd32
13 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.

@hakre
13 years ago

allow references in fields

#11 @hakre
13 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 13 years ago by hakre (previous) (diff)

#12 @hakre
13 years ago

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

@hakre
13 years ago

allow references in fields

#13 @chriscct7
9 years ago

  • Keywords needs-refresh dev-feedback added; has-patch removed

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

#14 @wonderboymusic
8 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to boonebgorges
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by sergey. View the logs.


8 years ago

#16 @wonderboymusic
8 years ago

  • Milestone changed from 4.4 to Future Release

#17 @dd32
6 years ago

  • Milestone changed from Future Release to 5.0
  • Owner changed from boonebgorges to dd32
  • Status changed from assigned to accepted

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

#18 @dd32
6 years ago

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

#19 @dd32
6 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 42527:

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

Fixes #16895.

#20 @johnbillion
5 years ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.