Make sure $list in wp_list_pluck() is an array

[josephscott]

I'm seeing wp_list_pluck() being used lots of times without checking to see if $list is really an array first. This results in many PHP warnings:

Invalid argument supplied for foreach()

Ideally code making use of wp_list_pluck() would make sure the parameters are the correct type first. Since that isn't happening it is time for wp_list_pluck() to take a more defensive stance.

The most direct path to addressing this is to cast $list to an array as part of the foreach process. I'll be adding a simple patch to this ticket that does this and adds a new unit test for this condition.

[dd32]

Ideally code making use of wp_list_pluck() would make sure the parameters are the correct type first.

For the record, I personally disagree with this proposal for that reason alone.

Since that isn't happening it is time for wp_list_pluck() to take a more defensive stance.

I think this just shows that allowing that warning to filter through is the right thing to do, instead of hiding it from developers, we should be letting them see it and act accordingly. It's annoying when you're working with someone else's code which isn't behaving, but at least you now know why the functionality you're looking at is potentially broken.

[nacin]

I strongly agree with @dd32 here.

The most that we'd do is check the type then want to issue a warning, rather than paper over the code. This is something we rarely do, and at that point, we might as well just keep the warning we have.

(FWIW: This sounds like a good opportunity for a unit test to assert that a non-array *does* throw a warning.)

[knutsp]

When I make this mistake, and I sometimes do, of calling the very handy wp_list_pluck() with a variable that in some cases may not be an array, I would certainly prefer to see a warning indicating my mistake, instead of just silently see, or miss, an empty result array. And which may make me think there was no data and start looking for them, instead of fixing my code.

[swissspidy]

This sounds like a good opportunity for a unit test to assert that a non-array *does* throw a warning.

35087.2.diff​ is a new patch that does exactly that. Will commit if it looks good.

[SergeyBiryukov]

Replying to swissspidy:

This sounds like a good opportunity for a unit test to assert that a non-array *does* throw a warning.

35087.2.diff​ is a new patch that does exactly that. Will commit if it looks good.

Looks good to me.

[swissspidy]

In 35949:

Ensure wp_list_pluck() throws a warning when not being passed an array.

We should not paper over the code and hide warnings from developers by casting values to an array.

See #35087.

[ocean90]

[35949] doesn't work as expected, see ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/97044732 and ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/97044733.

Test in different PHP versions: ​https://3v4l.org/9YsaD

[ocean90]

In 35950:

Revert [35949].

Passing an object to wp_list_pluck() throws also a fatal error, see ​https://3v4l.org/9YsaD.

See #35087.

[swissspidy]

Marking as wontfix (again), since the unit tests don't work with every PHP version.