Make WordPress Core

Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#56804 closed enhancement (fixed)

More readable and faster check for array of arrays in register_rest_route()

Reported by: tobiasbg's profile TobiasBg Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version: 6.1
Component: REST API Keywords: has-patch commit
Focuses: performance Cc:


In [54339] for #51986,

if ( count( array_filter( $arg_group['args'], 'is_array' ) ) !== count( $arg_group['args'] ) ) {

was used as a custom implementation of an any() function.

This is not very readable and not that nice performance-wise (array_filter() will always loop the full array, even though we are only interested in finding one (the first) non-array).

This should be simplified to a foreach loop that is left early once a match is found, for better readability and higher performance, e.g.:

foreach( $arg_group['args'] as $arg ) {
        if ( ! is_array( $arg ) ) {
                                /* translators: 1: $args, 2: The REST API route being registered. */
                                __( 'REST API %1$s should be an array of arrays. Non-array value detected for %2$s.' ),
                                '<code>' . $clean_namespace . '/' . trim( $route, '/' ) . '</code>'
                break; // Leave the foreach loop once one non-array argument was found.

(Follow-up to #51986 where this suggestion missed the deadline for 6.1-RC1, taking into account the linked-to Slack discussion in that ticket, regarding the target milestone. The Version of this ticket should be 6.1, but that does not yet exist in Trac.)

Change History (6)

This ticket was mentioned in PR #3453 on WordPress/wordpress-develop by @SergeyBiryukov.

19 months ago

  • Keywords has-patch added

#2 @SergeyBiryukov
19 months ago

  • Keywords commit dev-feedback added
  • Milestone changed from 6.1.1 to 6.1

Thanks for the ticket! The suggested code does look better to me in terms of readability and performance.

I believe this can be included in 6.1 RC2.

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

19 months ago

#4 @audrasjb
19 months ago

  • Keywords dev-feedback removed

As per today's bug scrub: @costdev and I both reviewed the proposed PR, and it looks like it's good to go, thanks!

#5 @SergeyBiryukov
19 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 54518:

REST API: Simplify the check for an array of arrays in register_rest_route().

The previous implementation of checking whether the args parameter is an array of arrays used array_filter(), which would always loop the full array, even though we are only interested in finding one (the first) non-array to display a _doing_it_wrong() message.

This commit aims to improve readability and performance of this check by using a foreach loop instead, leaving it as soon as the first non-array argument is found.

Follow-up to [54339].

Props TobiasBg, audrasjb, costdev, SergeyBiryukov.
Fixes #56804.

@SergeyBiryukov commented on PR #3453:

19 months ago

Merged in r54518.

Note: See TracTickets for help on using tickets.