Make WordPress Core

Opened 22 months ago

Closed 19 months ago

Last modified 3 months ago

#62558 closed feature request (fixed)

Add PHP 8.4 compat array functions: `array_find`, `array_find_key`, `array_any` and `array_all`

Reported by: Soean Owned by: desrosj
Priority: normal Milestone: 6.8
Component: General Version:
Severity: normal Keywords: has-patch php84 add-to-field-guide commit
Cc: Focuses: php-compatibility

Description

Add polyfill functions for PHP 8.4 array utilities

  • Added array_find function to search an array for the first element that passes a given callback.
  • Added array_find_key function to search an array for the first key that passes a given callback.
  • Added array_any function to check if any element of an array passes a given callback.
  • Added array_all function to check if all elements of an array pass a given callback.

Change History (12)

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


22 months ago
#1

  • Keywords has-patch added
  • Added array_find function to search an array for the first element that passes a given callback.
  • Added array_find_key function to search an array for the first key that passes a given callback.
  • Added array_any function to check if any element of an array passes a given callback.
  • Added array_all function to check if all elements of an array pass a given callback.

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

#2 @swissspidy
22 months ago

  • Focuses php-compatibility added
  • Milestone Awaiting Review6.8

#3 @TobiasBg
22 months ago

  • Keywords php84 added
  • Type defect (bug)feature request

Related: #62061, #62277
Previously added polyfills: [57337], [52038]

One Inline Documentation request, if I may: There should not be a blank line before the @return, see https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#1-functions-class-methods

#4 @TobiasBg
22 months ago

  • Keywords add-to-field-guide changes-requested added

#5 follow-up: @ayeshrajans
22 months ago

Do we have a guideline on which functions we polyfill, and which we don't?

PHP 8.4 has more new functions, and and the likelihood of functions like mb_*trim functions being useful enough to polyfill is as good as the new array_* functions.

I'm not objecting this ticket in particular, just wanted to put my two cents forward that we probably should follow a guideline (of which I don't know if exists) so we can maintain the polyfills properly.

#6 in reply to: ↑ 5 @desrosj
19 months ago

  • Keywords changes-requested removed

Replying to ayeshrajans:

Do we have a guideline on which functions we polyfill, and which we don't?

We don't currently have any official guidance in the handbook. Perhaps that's a good thing to add. If I had to write up that section today, this is what I would list as the criteria used to weigh when evaluating whether to include a PHP language polyfill:

  • Polyfill code size
  • How self contained is the polyfill? It should serve a single-responsibility unless absolutely necessary (we want to avoid adding entire polyfill classes and libraries as much as possible).
  • How many old code patterns does the polyfill replace in favor of consistency?
  • Performance improvement compared to these older patterns. Large improvements lend to the polyfill being a strong progressive enhancement that plugin and theme developers can count on being available.
  • Does the new function protect against common mistakes? For example, str_contains() guarding against scenarios where if ( strpos( 'This is a haystack', 'This' ) ) is evaluated as falsey.
  • Are these old patterns prevalent throughout Core? Would the polyfill help remove a good amount of duplicate code?
  • Maintenance burden of adding a polyfill. It should be simple to remove later.
  • The range of PHP versions currently supported. For example, if something is added in 8.4, a polyfill would currently allow for supporting the new function in 7 additional PHP versions.

In the case of these functions, it's difficult to find occurrence in Core where this would be useful, but I think it's a reasonable assumption there are a good number of instances where this would be useful. But it will require a more manual effort.

The PR looks good to me, I just approved it. I would love to give the opportunity for @joemcgill and @Mamaduka to weigh in as my fellow Core Tech Leads for 6.8 in case they have any opposing opinions.

#7 @desrosj
19 months ago

  • Keywords commit added

#8 @desrosj
19 months ago

  • Owner set to desrosj
  • Resolutionfixed
  • Status newclosed

In 59783:

General: Introduce polyfills for new array related functions in PHP 8.4.

PHP 8.4 introduced four new functions to provide a common way to more easily perform common operations on arrays.

These functions are now polyfilled making them available on all supported versions of PHP (currently 7.2+).

Props Soean, swissspidy, TobiasBg, ayeshrajans, mukesh27, joemcgill.
Fixes #62558.

#9 @Mamaduka
19 months ago

Reasoning makes sense. I use JS versions of these new methods daily, which are very handy.

I think we'll see more usage in the core now that polyfills have been merged.

#10 @dmsnell
3 months ago

I noticed this when seeing [62564] and wondered if we’re keeping a watchful eye on performance of these conversions. PHP still includes some disproportionately high cost for function calls, particularly user-space function calls, and we’re changing working code for styling purposes in ways that introduce new overhead on a substantial base of installed WordPresses.

The foreach form is not only about as efficient as possible, but it works with iterable values and ArrayAccess values while array_any() crashes on non-array inputs.

So these changes are not supposed to bring in behavioral changes, but do, increase the risk of PHP request crashes, and add overhead which isn’t much, but is also unnecessary (and given that it’s being applied to arrays, has a high chance of multiplying insignificant overhead into significant overhead).

I guess I feel like maybe we are jumping too quickly into changing existing code without a warrant that it needs updating, and putting existing working code at risk by assuming the updates are benign. I support having the polyfills, but the refactors like [62564] seem like they verge on change for the sake of change to the detriment of the project.

#11 @TobiasBg
3 months ago

@dmsnell: This comment is probably more applicable to #65519, right?

#12 @dmsnell
3 months ago

@TobiasBg thanks for asking. I think it’s appropriate in both places, given the discussion about guidelines on this ticket, and this ticket being the ancestor for the polyfills and refactors.

I’ve copied my comment into #65519 for completeness though.

Note: See TracTickets for help on using tickets.