WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 21 months ago

#43025 new defect (bug)

wp_list_filter should filter all iterable objects

Reported by: jarednova Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9.1
Component: General Keywords: has-unit-tests needs-patch
Focuses: Cc:
PR Number:

Description

#16499 introduced an issue which requires that data sent to wp_list_filter is an array. What it really means is the data sent to wp_list_filter should be iterable _like_ an array. By permitting both arrays and objects that are Transverable we can allow for anything that decends from the \ArrayObject class.

While there is an is_iterable method we _could_ use, it's only availabile in PHP 7.1 so the attached is friendly to PHP v4.2 and above

Attachments (3)

43025.diff (503 bytes) - added by jarednova 22 months ago.
Proposed patch
43025.test.diff (695 bytes) - added by jarednova 22 months ago.
Test for patch
43025.2.diff (2.4 KB) - added by jarednova 21 months ago.
Reaction to feedback on patch

Download all attachments as: .zip

Change History (9)

@jarednova
22 months ago

Proposed patch

#1 @jarednova
22 months ago

  • Keywords has-patch added

#2 @swissspidy
22 months ago

  • Keywords needs-unit-tests added

@jarednova
22 months ago

Test for patch

#3 @jarednova
22 months ago

@swissspidy I added a test in 43025.test.diff — please let me know if you need anything else to make your review as easy as possible!

#4 @swissspidy
21 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@flixos90 Since you worked on WP_List_Util, would you mind having a look?

I think we can add such a check directly to the class instead of the helper function.

Also, I'd use instanceof instead of is_a(). We rarely use the latter in core and it's better to be consistent.

#5 @flixos90
21 months ago

  • Keywords needs-patch added; has-patch removed

Some thoughts from my end:

  • I agree with @swissspidy that the is_array() clauses in the beginning of wp_list_filter() (and wp_list_sort() and wp_filter_object_list() as well!) should be removed from there and become part of WP_List_Util. The easiest way would be to check in the constructor whether $input is an array of traversable, and if not, simply set it to an empty array.
  • I agree that instanceof should be used instead of is_a(). Also recommended since it's a language construct and not a function.
  • Not sure whether that's necessary, but maybe we should adjust all related docblocks to say "iterable" instead of array.
  • And now it gets a little more complex: WP_List_Util::filter() and WP_List_Util::sort() create a new array with the data from the input. Not sure whether we should just keep it that way or try to return the same object that was passed (that would probably make sense). If yes, how could we achieve this reasonably? Add a private utility method that transforms the output array back to the same type of traversable object if the input is one? Should we use clone and then populate the cloned object with the new values?
  • WP_List_Util::sort() uses usort() and uasort() both of which don't work with a traversable object. Maybe we really need to transform the traversable into an array immediately and transform it back after the operations.

IMO this is a little more complex than initially thought.

Last edited 21 months ago by flixos90 (previous) (diff)

@jarednova
21 months ago

Reaction to feedback on patch

#6 @jarednova
21 months ago

@flixos90 I resolved some of the points you raised in 43025.2.diff...

I agree with @swissspidy that the is_array() clauses in the beginning of wp_list_filter() (and wp_list_sort() and wp_filter_object_list() as well!) should be removed from there and become part of WP_List_Util. The easiest way would be to check in the constructor whether $input is an array of traversable, and if not, simply set it to an empty array.

I created a function called WP_List_Util::maybe_list() to take an input and determine whether it's a valid iterable object. If it's not, it just sends back an empty array. I took a stab at the naming here — would love your thoughts on the name and whether this matches with what you were looking for.


I agree that instanceof should be used instead of is_a(). Also recommended since it's a language construct and not a function.

Done!


Not sure whether that's necessary, but maybe we should adjust all related docblocks to say "iterable" instead of array.
And now it gets a little more complex: WP_List_Util::filter() and WP_List_Util::sort() create a new array with the data from the input. Not sure whether we should just keep it that way or try to return the same object that was passed (that would probably make sense). If yes, how could we achieve this reasonably? Add a private utility method that transforms the output array back to the same type of traversable object if the input is one? Should we use clone and then populate the cloned object with the new values?
WP_List_Util::sort() uses usort() and uasort() both of which don't work with a traversable object. Maybe we really need to transform the traversable into an array immediately and transform it back after the operations.

Like you said, this "more complex" stuff I left for now

Note: See TracTickets for help on using tickets.