WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#35087 closed defect (bug) (wontfix)

Make sure $list in wp_list_pluck() is an array

Reported by: josephscott Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

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.

Attachments (2)

35087.diff (1.2 KB) - added by josephscott 3 years ago.
35087.2.diff (938 bytes) - added by swissspidy 3 years ago.

Download all attachments as: .zip

Change History (12)

@josephscott
3 years ago

#1 @dd32
3 years ago

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.

#2 @nacin
3 years ago

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

#3 @knutsp
3 years ago

  • Keywords close added

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
3 years ago

#4 follow-up: @swissspidy
3 years ago

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.

#5 in reply to: ↑ 4 @SergeyBiryukov
3 years ago

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.

#6 @swissspidy
3 years ago

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.

#7 @swissspidy
3 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

#8 @ocean90
3 years ago

  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

#9 @ocean90
3 years ago

In 35950:

Revert [35949].

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

See #35087.

#10 @swissspidy
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.