Make WordPress Core

Opened 2 years ago

Closed 4 months ago

Last modified 4 months ago

#55105 closed feature request (fixed)

Introduce a polyfill for `array_is_list()`.

Reported by: costdev's profile costdev Owned by:
Milestone: 6.5 Priority: normal
Severity: minor Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

PHP 8.1 introduces a new function, array_is_list().

The function determines whether an array's keys consist of consecutive numbers from 0 to count( $arr ) - 1.

Including a polyfill will allow developers to take advantage of the new function.

Notes

  • A list must start from an offset of 0. array( 2 => 'one', 3 => 'two' ); will return false.
  • An empty array will return true.

References

Attachments (1)

55105.diff (3.8 KB) - added by costdev 2 years ago.

Download all attachments as: .zip

Change History (27)

#1 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

This ticket was mentioned in PR #2289 on WordPress/wordpress-develop by costdev.


2 years ago
#2

  • Keywords has-patch has-unit-tests added

#3 @costdev
2 years ago

PR ready for review.

#4 @rafiahmedd
2 years ago

@costdev sorry if I am wrong, I think on line 500 it will be if( $k !== $i ) then return false.

#5 follow-up: @costdev
2 years ago

Hi @rafiahmedd, thanks for reviewing the PR! I left a comment there about pre-incrementation vs post-incrementation.

#6 in reply to: ↑ 5 @rafiahmedd
2 years ago

Replying to costdev:

Hi @rafiahmedd, thanks for reviewing the PR! I left a comment there about pre-incrementation vs post-incrementation.

@costdev thank you brother 😊

#7 @jrf
2 years ago

I'm wondering why this function should be polyfilled. Can you give one or more examples of code in WordPress which would benefit from the function being available ?

If there are no current usecases for this function in Core, the function shouldn't be introduced.

#8 @davidbaumwald
2 years ago

  • Keywords dev-feedback added

#9 @costdev
2 years ago

@jrf I tried to find a use case prior to writing the polyfill. I couldn't find one. Examples online seem to literally just reiterate what the function does, without providing use cases, and vaguely referring to "PHP security vulnerabilities caused by arrays that are not in order", without mentioning array_values() once.

Why did I write it then?

I'd just reviewed and written tests for some of the polyfills introduced in 5.9, and I wasn't quite done with my polyfilling-spree. It's there, with unit tests, if use cases are found.

As we're still early enough in the release cycle, I'll leave this open in case someone sees value in introducing this polyfill to Core. If there are none provided as we approach feature freeze, I'll go ahead and close as maybelater, or punt to 6.1 if use cases are actively being discussed.

#10 @peterwilsoncc
2 years ago

  • Version 5.9 deleted

Feature requests don't require a version, so I have removed that.

@costdev As there hasn't been any additional discussion, are you able to decide whether you'd like to have another go in a future milestone or close this as maybelater?

#11 follow-up: @costdev
2 years ago

  • Keywords dev-feedback removed
  • Milestone 6.0 deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Yeah, let's close this one as maybelater so it can be picked up, with unit tests, if use cases are found.

For some futureproofing, I'll close the PR and upload a .diff to the ticket, which will include an additional dataset for array( NAN ).

Some history for contributors who may look at this in future:

  1. The patch uses foreach ( $arr... instead of array_keys( $arr ) or array_keys($arr) === range(count($arr)-1) to avoid creating new arrays unnecessarily. This means only one pass over the array, and is in line with the example given in the RFC's Proposal section.
  2. An early return for an empty array is not included because an empty array will skip on the foreach() and return true.
  3. An alternative implementation used by Symfony has an early return on $array === array_values($array). The performance improvement from this early return is negligible, but importantly, performance is inconsistent across different versions of PHP, so I think the current implementation is the simplest and most effective polyfill for array_is_list().

@costdev
2 years ago

costdev commented on PR #2289:


2 years ago
#12

Closed in favour of 55105.diff.

#13 @johnjamesjacoby
19 months ago

Some random related thoughts...


See also: wp_is_numeric_array() & rest_is_array() for similar but different examples of functions in WordPress that are documented to imply their intention may have been to mimic the behavior of array_is_list() but perhaps a consult from @rmccue would be helpful?

The above helper function(s) @return bool Whether the variable is a list but do not enforce @costdev's first point:

A list must start from an offset of 0

(emphasis, mine... but I think I agree?)

While a bunch of other programming languages explicitly define & strictly enforce "List" and "Array" as different things (C, Java, Python, etc...) neither PHP nor WordPress do currently.

I think that means it is safe to suggest that WordPress functions named like wp_parse_list() and wp_list_*() – and the WP_List_Util() class – largely imply that they are intended to work with either arrays or objects. Similarly, only the REST API (and later the Meta API via default values in get_metadata_default()) have experienced a need for identifying whether a variable value was a numerically indexed array that starts with 0 and does not omit any subsequent integers as keys until the final key.


Without speculating too deeply, PHP core is trending towards becoming more strict (hence more static), so perhaps someday a "List" could be a more explicitly defined thing, and it may behoove WordPress core contributors to have a plan for that and decide whether to:

  • Keep down the path of generalized "lists" as arrays or objects
  • Add to the global namespace list functions & classes that mean what array_is_list() means

#14 @swissspidy
8 months ago

The Performant Translations feature plugin is one project that uses array_is_list() (ref) and would require this polyfill upon core merge.

#15 in reply to: ↑ 11 @SergeyBiryukov
8 months ago

  • Milestone set to 6.5
  • Resolution maybelater deleted
  • Status changed from closed to reopened

Replying to costdev:

Yeah, let's close this one as maybelater so it can be picked up, with unit tests, if use cases are found.

It is worth noting that polyfills for array_key_first() and array_key_last() were added in [52038] / #45055 even without being used by WordPress core.

I believe the benefit here is pretty much the same as in that ticket:

Including a polyfill would allow developers to take advantage of the new functions and write consistent code regardless of a site's PHP version.

Now that WordPress has PHP 8.1 compatibility (with exceptions), I think it's time to reconsider this.

With 6.4 Beta 1 coming in a few days, moving to 6.5 for discussion.

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


4 months ago

#17 @swissspidy
4 months ago

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

A polyfill for this actually got included as part of #59656, see r57337. Marking as fixed.

#18 @jrf
4 months ago

@swissspidy You may want to prep a PR for the PHPCompatibilityWP ruleset in that case. While use of the function won't be flagged by the latest release, it will be once PHPCompatibility 10 comes out.

#19 @costdev
4 months ago

@swissspidy The polyfill in r57337 is exponentially less performant than the one proposed in this ticket. array_values() is O(n), for example, and it also includes a second passover that I wasn't able to find a scenario for. See this thread for where I looked at Symfony's polyfill and also mention the inconsistent performance of array_values().

The polyfill in r57337 also has no tests, which is common practice for polyfills.

I'd suggest that we consider the polyfill in this ticket's PR and its accompanying tests to replace the polyfill in that changeset.

#20 @swissspidy
4 months ago

@jrf Will do!

@costdev Thanks for raising this.

Happy to add the tests for sure.

Looking at https://3v4l.org/tgUEX/perf and https://3v4l.org/MLs3Z/perf for the relevant version range (PHP 7.0 to PHP 8.0), user time is identical. Maybe the performance inconsistencies were a thing before WP bumped its minimum requirement? It does look like implementing both approaches à la Symfony is redundant, so it makes sense to just settle on one (assuming the tests confirm that)

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


4 months ago
#21

The function was polyfilled via another ticket.

This adds tests that were originally added and simplifies the polyfill (assuming the tests still pass)

Trac ticket: https://core.trac.wordpress.org/ticket/55105

@swissspidy commented on PR #6028:


4 months ago
#22

Okay so the shorter version fails for the array(NAN) case on PHP 7.0 and PHP 7.1

Benchmarking with a larger array now. Turns out the array_values() approach is faster for the happy path, see https://3v4l.org/EMpT0/perf (foreach) vs https://3v4l.org/1WsAV/perf (array_values())

For the unhappy path the performance seems equal, see https://3v4l.org/5YMl2/perf (foreach), https://3v4l.org/o1SSY/perf (array_values()) and https://3v4l.org/H721m/perf (both).

So that appears to make a case for using both as Symfony does.

@costdev commented on PR #6028:


4 months ago
#23

Not sure how both can ever be run though. If array_values( $arr ) === $arr, then when would the foreach() run?

@swissspidy commented on PR #6028:


4 months ago
#24

@costdev It's specifically that the output for using array( NAN ) as the input is different on PHP 7.0 and PHP 7.1. It doesn't really make sense to me.

See https://3v4l.org/nPN8l

#25 @swissspidy
4 months ago

In 57535:

General: Add tests for array_is_list polyfill added in r57337.

Props costdev.
See #55105.

Note: See TracTickets for help on using tickets.