WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#37128 closed enhancement (fixed)

Introduce helper function `wp_list_sort()`

Reported by: flixos90 Owned by: swissspidy
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch needs-dev-note has-unit-tests commit
Focuses: Cc:

Description

When dealing with object lists in WordPress, we have easy-to-use utility functions available for filtering (wp_list_filter()) and plucking a field (wp_list_pluck()). What we don't have however, is a function to easily sort the list by a specific field.

Although a function like this wouldn't need to be used inside of WordPress Core, it would certainly make it easier for plugin developers by being provided such a function. Therefore I think we should add it.

Attachments (11)

37128.diff (2.5 KB) - added by flixos90 3 years ago.
37128.2.diff (11.3 KB) - added by flixos90 3 years ago.
37128.3.diff (23.6 KB) - added by swissspidy 3 years ago.
37128-usage.diff (4.5 KB) - added by flixos90 3 years ago.
Usage examples for wp_list_sort() in Core
37128-usage.2.diff (5.3 KB) - added by flixos90 3 years ago.
37128-usage.3.diff (10.8 KB) - added by flixos90 3 years ago.
active-sections-after-r38859.png (68.7 KB) - added by westonruter 3 years ago.
active-sections-before-r38859.png (137.1 KB) - added by westonruter 3 years ago.
37128.4.diff (34.8 KB) - added by flixos90 3 years ago.
37128.5.diff (40.1 KB) - added by flixos90 3 years ago.
37128.6.diff (42.4 KB) - added by flixos90 3 years ago.

Download all attachments as: .zip

Change History (50)

@flixos90
3 years ago

#1 @flixos90
3 years ago

  • Keywords has-patch added; needs-patch removed

37128.diff adds a function wp_list_sort().

The patch also adds another private function _wp_list_sort_callback() which is used by usort() in the main function. The fields to order by are temporarily stored in a private global $_wp_list_sort which is only available while wp_list_sort() is doing its work.

As far as the parameters work, they are similar like the $orderby and $order arguments of WP_Query for example. One can either pass $orderby as a string (field name) and $order (ASC or DESC) or alternatively only specify $orderby as an array of orderby => order to support sorting by multiple fields.

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


3 years ago

#3 @swissspidy
3 years ago

Using globals is bad, using temporary globals seems even worse. With initiatives like #37699, we should avoid introducing new globals.

How would this look with a helper class for plucking, filtering and sorting? Or maybe create_function() (sigh)?

Introducing wp_list_sort() without using it in core is not ideal, but an extensible helper class for interacting with arrays and objects might be more convincing.

@flixos90
3 years ago

#4 @flixos90
3 years ago

  • Keywords needs-unit-tests added

@swissspidy: Thanks for the feedback, good point. In 37128.2.diff I introduce a new class WP_List_Util that takes care of that functionality. It also takes care of filtering and plucking, so the original functions only act as wrappers now. This way we have a clear structure for also performing multiple operations on one list.

#5 @DrewAPicture
3 years ago

  • Milestone changed from Awaiting Review to 4.7

WP_List_Util is certainly interesting. I think I'd like to see us bail early in the filter() method if the $operator doesn't match a whitelist. Also still needs unit tests :-)

@swissspidy
3 years ago

#6 @swissspidy
3 years ago

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

37128.3.diff adds unit tests and bails early in filter() if the $operator is not and, or or not.

#7 follow-up: @jorbin
3 years ago

In general, core has tended to not add utility functions that it doesn't use. Is there anywhere in core this would make sense right now? Not saying that should keep it out, but I think if core can use it, the case for including it is stronger.

One thing that isn't obvious from me just reading the tests and the docs is what happens if wp_list_sort is called on an object where $orderby isn't set on each field? Where does that fall in the sort? Seems like that is something that should be documented and tested so the code remains consistent into the future.

Additionally, what about when you want to do both asc and desc ? For example, alphabetical by title, but reverse chronological? Perhaps there should just be a single argument that you pass in that includes both the field and the direction?

#8 in reply to: ↑ 7 ; follow-up: @flixos90
3 years ago

Replying to jorbin:

In general, core has tended to not add utility functions that it doesn't use. Is there anywhere in core this would make sense right now? Not saying that should keep it out, but I think if core can use it, the case for including it is stronger.

Good point, I was actually planning to look for use-cases earlier. I just checked and found several places where using wp_list_sort() would make sense. We could get rid of the following functions/methods (or at least not use them anymore):

  • _usort_terms_by_name()
  • _usort_terms_by_ID()
  • _sort_nav_menu_items()
  • WP_Customize_Manager::_cmp_priority()
  • WP_Customize_Widgets::_sort_name_callback()

There might be more, but that's what I found now. So there would indeed be a use for the function in Core. I can update another patch later today or tomorrow to illustrate what the implementation could look like.

One thing that isn't obvious from me just reading the tests and the docs is what happens if wp_list_sort is called on an object where $orderby isn't set on each field? Where does that fall in the sort? Seems like that is something that should be documented and tested so the code remains consistent into the future.

If the $orderby field isn't set on one of the objects/arrays compared, the comparison is skipped, basically resulting in an equal comparison. In this case the next field compared (if using an array of $orderby => $order) will decide. If all comparisons between two objects result in an equal check, the whole check will cause an equal result (i.e. return 0). Since PHP only does unstable sort, this will unfortunately randomize the result. However we can improve this if needed, for example by adding an internal __index property on each item in the array (and removing it again after the comparison) and then fallback to it to maintain the original order on an equal comparison.

Additionally, what about when you want to do both asc and desc ? For example, alphabetical by title, but reverse chronological? Perhaps there should just be a single argument that you pass in that includes both the field and the direction?

In the patch you can do that already: you can specify the $orderby parameter as an array of (multiple) $orderby => $order pairs (in this case it will skip the third $order parameter). Is that what you meant or did I misunderstand you?

#9 in reply to: ↑ 8 ; follow-up: @swissspidy
3 years ago

Replying to flixos90:

One thing that isn't obvious from me just reading the tests and the docs is what happens if wp_list_sort is called on an object where $orderby isn't set on each field? Where does that fall in the sort? Seems like that is something that should be documented and tested so the code remains consistent into the future.

If the $orderby field isn't set on one of the objects/arrays compared, the comparison is skipped, basically resulting in an equal comparison. In this case the next field compared (if using an array of $orderby => $order) will decide. If all comparisons between two objects result in an equal check, the whole check will cause an equal result (i.e. return 0). Since PHP only does unstable sort, this will unfortunately randomize the result. However we can improve this if needed, for example by adding an internal __index property on each item in the array (and removing it again after the comparison) and then fallback to it to maintain the original order on an equal comparison.

I don't really think we should go as far as adding internal properties and such and simply let PHP handle this.

Additionally, what about when you want to do both asc and desc ? For example, alphabetical by title, but reverse chronological? Perhaps there should just be a single argument that you pass in that includes both the field and the direction?

In the patch you can do that already: you can specify the $orderby parameter as an array of (multiple) $orderby => $order pairs (in this case it will skip the third $order parameter). Is that what you meant or did I misunderstand you?

What about removing that third parameter and always requiring a key => value pair?

#10 in reply to: ↑ 9 @flixos90
3 years ago

Replying to swissspidy:

What about removing that third parameter and always requiring a key => value pair?

I was aiming for parity with how all the *_Query classes handle their order arguments, but I'm certainly open to remove that parameter.

@flixos90
3 years ago

Usage examples for wp_list_sort() in Core

#11 @flixos90
3 years ago

37128-usage.2.diff includes some use-cases implemented for wp_list_sort() in Core. When testing make sure to also merge the patch for the actual functionality (since this patch only includes implementation).

#12 follow-up: @swissspidy
3 years ago

37128-usage.2.diff should also remove/deprecate the now unneeded sorting functions in these files (dead code).

#13 in reply to: ↑ 12 ; follow-up: @flixos90
3 years ago

Replying to swissspidy:

37128-usage.2.diff should also remove/deprecate the now unneeded sorting functions in these files (dead code).

Deprecate or remove? I would assume we can remove the ones that are private/protected (although they could theoretically be used by someone extending a class).

#14 in reply to: ↑ 13 @swissspidy
3 years ago

Replying to flixos90:

Replying to swissspidy:

37128-usage.2.diff should also remove/deprecate the now unneeded sorting functions in these files (dead code).

Deprecate or remove? I would assume we can remove the ones that are private/protected (although they could theoretically be used by someone extending a class).

Private methods could be removed straight away (not inherited), but at first glance it looks like these are only public methods. So maybe deprecating them first before removing them early in the next release.

#15 @flixos90
3 years ago

In 37128-usage.3.diff all previously used methods and functions have been deprecated. All of these are either functions marked private or protected methods, so we can probably remove them quickly, like one release after the deprecated notice. Still, for now all of them remain and are only deprecated.

What would be the next steps for this ticket? Anyone who would be willing to review this one? :)

#16 @swissspidy
3 years ago

If @DrewAPicture can approve the docs / changes in 37128-usage.3.diff, I'm happy to commit this :-)

#17 @swissspidy
3 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

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


3 years ago

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


3 years ago

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


3 years ago

#21 @jorbin
3 years ago

  • Keywords commit added

@swissspidy Let's land it.

#22 @swissspidy
3 years ago

  • Keywords needs-dev-note added

#23 @swissspidy
3 years ago

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

In 38859:

General: Introduce a wp_list_sort() helper function.

In addition to wp_list_filter() for filtering a list of objects, and wp_list_pluck() for plucking a certain field out of each object in a list, this new function can be used for sorting a list of objects by specific fields. These functions are now all contained within the new WP_List_Util() class and wp_list_sort() is used in various parts of core for sorting lists.

Props flixos90, DrewAPicture, jorbin.
Fixes #37128.

#24 @ocean90
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@flixos90, @swissspidy There are a few failing tests, see https://travis-ci.org/aaronjorbin/develop.wordpress/builds/169534080.

#25 @westonruter
3 years ago

In 38862:

Customize: Revert part of [38859] which caused sections to get deactivated in the customizer.

See #37128.

#26 @westonruter
3 years ago

See active-sections-before-r38859.png and active-sections-after-r38859.png for what the partial revert in [38862] fixed. There was an issue in [38859] where the new sorting function was corrupting the list of sections.

#27 @swissspidy
3 years ago

@westonruter Thanks for the screenshots. wp_list_sort() right now only does a usort(), whereas the affected customizer parts sometimes use uasort(). [38862] reverts an instance of usort() as well though. With that being said, I'd rather have reverted the whole commit because of the test failures instead.

The tests are failing because usort() / uasort() are not stable and their behaviour changed from PHP 5 to PHP 7. That makes it basically untestable.

What we definitely need is wp_list_usort() and wp_list_uasort() or an additional parameter to wp_list_sort().

#28 @jorbin
3 years ago

@swissspidy Can you finish the revert and we can continue work on this as a patch?

#29 @ocean90
3 years ago

In 38863:

Revert [38859] due to an incomplete implementation.

See https://core.trac.wordpress.org/ticket/37128#comment:27.
See #37128.

@flixos90
3 years ago

#30 @flixos90
3 years ago

37128.4.diff fixes the failing unit tests (related to PHP unstable sorting). It also adds a $preserve_keys parameter to wp_list_sort() (if true, uasort() is used). This parameter is provided as such in the relevant functions in WP_Customize_Manager.

Note that this patch now contains both the basic implementation plus the usage in the rest of Core to make maintenance easier.

#31 @ocean90
3 years ago

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

Travis CI build for 37128.4.diff: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/169711013 (passed)

Can we have a test which covers [38862]?

#32 @flixos90
3 years ago

@ocean90 Will do that later this weekend.

@flixos90
3 years ago

#33 @flixos90
3 years ago

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

37128.5.diff adds a unit test test_wp_list_sort_preserve_keys() to test the new $preserve_keys functionality (using uasort()).

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


3 years ago

#35 follow-up: @swissspidy
3 years ago

Travis CI of the latest patch looks good: https://travis-ci.org/aaronjorbin/develop.wordpress/builds/170390880.

@westonruter Wanna test this with the customizer again?

#36 in reply to: ↑ 35 @ocean90
3 years ago

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

Replying to swissspidy:

@westonruter Wanna test this with the customizer again?

Said this already to @flixos90: The test_wp_list_sort_preserve_keys() is good but my intention was to have a test which covers the sorting behaviour for the customizer specifically.

@flixos90
3 years ago

#37 @flixos90
3 years ago

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

In 37128.6.diff I added three unit tests for the customizer that checks that the sorted objects still have valid keys for controls, sections and panels respectively.

#38 @swissspidy
3 years ago

  • Keywords commit added

#39 @ocean90
3 years ago

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

In 38928:

General: Introduce a wp_list_sort() helper function, v2.

In addition to wp_list_filter() for filtering a list of objects, and wp_list_pluck() for plucking a certain field out of each object in a list, this new function can be used for sorting a list of objects by specific fields. These functions are now all contained within the new WP_List_Util() class and wp_list_sort() is used in various parts of core for sorting lists.

This was previously committed in [38859] but got reverted in [38862] and [38863]. To fix the previous issues, wp_list_sort() supports now an additional argument to preserve array keys via uasort().

Props flixos90, DrewAPicture, jorbin.
Fixes #37128.

Note: See TracTickets for help on using tickets.