wp_list_pluck unexpectedly returns id indexed array instead of plucked values with index_key = null

[adamsilverstein]

Recently cane across an interesting bug. We were grabbing a list of term slugs, using the following code:

$term_slugs = wp_list_pluck( get_the_terms( $ID, 'category' ), 'slug' );

As a result of a recently patched bug, get_the_terms' cached return was an array that used term ID as the index for each term object, instead of the usual 'zero-indexed' (or sequential) array.

Interestingly, when this indexed array was passed to wp_list_pluck, the return was not the expected array of values. Instead, wp_list_pluck returned an indexed array of values, what you would expect if the function had been called with $index_key = 'term_id'.

wp_list_pluck should "Pluck a certain field out of each object in a list." and shouldn't care if the list is indexed; we should always expect an array of the found values unless the $index_key option is set (matching the behaviour of PHP's array_column).

[adamsilverstein]

fix wp_list_pluck for indexed arrays

[adamsilverstein]

31316.diff​ includes a unit test and fix for the issue.

[adamsilverstein]

Introduced in r15686

[dd32]

It's worth mentioning here, that altering wp_list_pluck() could be out of the question based on plugins (or core) expecting this behaviour.

It might be better to raise this as a bug in get_the_terms() if the return values changed.

[adamsilverstein]

Replying to dd32:

It's worth mentioning here, that altering wp_list_pluck() could be out of the question based on plugins (or core) expecting this behaviour.

It might be better to raise this as a bug in get_the_terms() if the return values changed.

Thanks for the feedback, understood we can't go around messing with core functions recklessly, however this seems like buggy or at the very least unexpected behaviour.

There is already a ticket for the get_the_terms cache bug, (https://core.trac.wordpress.org/ticket/31086). If we can't change the unexpected behaviour, second bet would be to document it so its expected.

[adamsilverstein]

re: "plugins (or core) expecting this behaviour." - I'm not sure why they would expect this bahaviour, its completely unexpected - thats not what the function doc block states the function is going to return.

Replying to dd32:

It's worth mentioning here, that altering wp_list_pluck() could be out of the question based on plugins (or core) expecting this behaviour.

It might be better to raise this as a bug in get_the_terms() if the return values changed.

[dd32]

Replying to adamsilverstein:

re: "plugins (or core) expecting this behaviour." - I'm not sure why they would expect this bahaviour, its completely unexpected - thats not what the function doc block states the function is going to return.

Regardless of what the Docblock suggests, it's what the code explicitly did (by using $key) and how it acted. Documentation can be wrong, and unfortunately in the past WordPress's documentation has lacked in both details and matching the exact output from functions, while we have far better documentation now, it's still possible there's bugs.

For example, look at how it was when introduced in 3.1, the documentation remained the same until 4.0 when return docs were added in r28936.

[boonebgorges]

I came here to say that we probably can't change this for backward compatibility reasons, but dd32 beat me to it :) I don't disagree that the behavior is a bit odd with numeric keys, but it's very likely that there are plugins that are expecting to have the keys preserved. In the case of associative arrays, it would be bad indeed if this behavior were changed.

It's worth noting that PHP's array_filter() preserves keys in the same way as wp_list_pluck(), but array_column() does not.

I do think it's worth adding a line to the docblock explaining the current behavior.

[adamsilverstein]

Thanks for the input Boone. I understand the backwards compatibility argument; and together you have certainly convinced me!

I guess its a feature, not a bug :) I will prepare a docblock patch to fix the current incorrect implication.

An easy workaround if you always want the values only is to run the array thru array_values() before wp_list_pluck.

Note that previous to r15686, the wp_filter_object_list function would have only returned the values from an array (even if an index were present), unless a $field were specified; when the code was split off into wp_list_pluck (and wp_list_filter), it was changed to include the index with the return (if present in the original array).

Replying to boonebgorges:

I came here to say that we probably can't change this for backward compatibility reasons, but dd32 beat me to it :) I don't disagree that the behavior is a bit odd with numeric keys, but it's very likely that there are plugins that are expecting to have the keys preserved. In the case of associative arrays, it would be bad indeed if this behavior were changed.

It's worth noting that PHP's array_filter() preserves keys in the same way as wp_list_pluck(), but array_column() does not.

I do think it's worth adding a line to the docblock explaining the current behavior.

[adamsilverstein]

docblock update, 1st pass

[adamsilverstein]

31316.3.diff​: Docblock update, take one.

[boonebgorges]

Thanks, adamsilverstein.

[boonebgorges]

In 31451:

Improve documentation for return value of wp_list_pluck().

wp_list_pluck() will preserve the original array keys if no $index_key
parameter is provided. This changeset updates the documentation accordingly.

Props adamsilverstein.
Fixes #31316.