Make WordPress Core

Opened 5 weeks ago

Last modified 45 hours ago

#65842 reopened enhancement

Use isset() instead of in_array() over array_keys() for key lookups

Reported by: mukesh27 Owned by: mukesh27
Priority: normal Milestone: 7.2
Component: General Version:
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses: performance

Description

Several places in core test whether an array has a given key by building the full key list and scanning it linearly:

in_array( $key, array_keys( $array ), true )

This allocates a complete copy of every key and walks it — O(n) time and O(n) memory — where isset( $array[ $key ] ) does a single hash lookup in constant time with no allocation.

Change History (8)

#1 @mukesh27
5 weeks ago

  • Owner set to mukesh27
  • Status newassigned

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


5 weeks ago
#2

  • Keywords has-patch added; needs-patch removed

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

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude
Model(s): Opus 5
Used for: Initial code skeleton and test suggestions; final implementation and tests were reviewed and edited by me.

#3 follow-up: @siliconforks
5 weeks ago

Note that in some cases isset might not give the same result as in_array. For example, if the value of the array element is null, isset will return false.

<?php

$array = [
        'foo' => 1,
        'bar' => null,
        'baz' => 3,
];

$key = 'bar';

if ( in_array( $key, array_keys( $array ), true ) ) {
        echo 'in_array: true';
} else {
        echo 'in_array: false';
}
echo "\n";

if ( isset( $array[ $key ] ) ) {
        echo 'isset: true';
} else {
        echo 'isset: false';
}
echo "\n";

Running the above code gives:

in_array: true
isset: false

If you are certain that $array will never contain null values, then this is not an issue; but if you are not certain of this, it might be safer to use array_key_exists instead. (See the second example in the documentation for array_key_exists.)

#4 in reply to: ↑ 3 @SergeyBiryukov
5 weeks ago

  • Milestone Future Release7.2

Replying to siliconforks:

Note that in some cases isset might not give the same result as in_array. For example, if the value of the array element is null, isset will return false.

Good point, thanks! I have checked all the instances in the attached PR and I think they should never contain null, so isset seems safe here.

#5 @SergeyBiryukov
5 weeks ago

  • Resolutionfixed
  • Status assignedclosed

In 63177:

Code Quality: Use isset() instead of in_array() over array_keys() for key lookups.

This updates several places in core that test whether an array has a given key:

  • in_array( $key, array_keys( $array ), true ) allocates a complete copy of every key and walks it.
  • isset( $array[ $key ] ), on the other hand, does a single hash lookup in constant time with no allocation.

Note: For arrays that main contain null values, array_key_exists() should be used instead.

Developed in https://github.com/WordPress/wordpress-develop/pull/12929.

Props mukesh27, siliconforks.
Fixes #65842.

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


4 weeks ago
#6

  • Keywords has-unit-tests added

## What?

[63177] replaced in_array( $size, array_keys( $_wp_additional_image_sizes ), true ) with isset( $_wp_additional_image_sizes[ $size ] ) in image_constrain_size_for_editor().

The two forms are not equivalent when $size is not a valid array key type. This PR guards the lookup so that such values fall through to the unconstrained branch, as they did before [63177].

- } elseif ( isset( $_wp_additional_image_sizes[ $size ] ) ) {
+ } elseif ( ( is_string( $size ) || is_int( $size ) ) && isset( $_wp_additional_image_sizes[ $size ] ) ) {

## Why?

in_array( ..., true ) accepts any value and simply returns false for one that could never be an array key. Using that same value as an array offset does not:

$size Before [63177] After [63177]
null false Deprecated: Using null as an array offset is deprecated, use an empty string instead (PHP 8.5)
1.5 false Deprecated: Implicit conversion from float 1.5 to int loses precision
new stdClass false TypeError: Cannot access offset of type stdClass in isset or empty
resource false Warning: Resource ID#n used as offset, casting to integer

The isset() behaviour is confirmed by PHP's own test, Zend/tests/isset/isset_array.phpt. The null case is the PHP 8.5 deprecation of null as an array offset, which WordPress now runs in CI.

$size reaches this line unfiltered from public API:

wp_get_attachment_image_url()
  → wp_get_attachment_image_src()
    → image_downsize()
      → image_constrain_size_for_editor()

Only is_array() is handled earlier, at the top of the function.

Passing null as an image size is invalid per the documented string|int[] type, but it is common in the wild — typically wp_get_attachment_image_src( $id, $atts['size'] ?? null ) and similar — and it was silently harmless before [63177].

It was caught by the Gutenberg plugin's PHP 8.5 CI running against Core trunk, where the Cover block passed $attributes['sizeSlug'] ?? null:

Tests_Blocks_Render_Cover::test_gutenberg_render_block_core_cover
Using null as an array offset is deprecated, use an empty string instead

That call site is now fixed in Gutenberg (WordPress/gutenberg#81444), but that fixes one caller. The behaviour change in Core affects all of them. Core's bundled copy in src/wp-includes/blocks/cover.php still passes null and will pick the fix up on the next Gutenberg package sync.

## Why this guard?

string and int are exactly the two types PHP accepts as array keys, so the guard expresses the precondition the isset() lookup actually has: is this value usable as an offset at all? Both checks are constant-time, so the improvement from [63177] is preserved.

is_string() alone would cover every realistic case, since add_image_size() documents $name as string. But a size registered with a purely numeric name gets its array key cast to int, and the pre-[63177] in_array( ..., true ) did match an int argument against it, so including is_int() avoids quietly narrowing that.

No lower bound is applied to the int case. Negative integers are valid array keys and produce no diagnostic on PHP 8.5 — verified on 8.5.9:

$sizes = array( 'test-size' => array( 'width' => 300 ), -5 => array( 'width' => 100 ) );
var_dump( isset( $sizes[-5] ) );  // bool(true), no notice

So a $size >= 0 condition would add nothing for PHP 8.5 safety while excluding a size registered as add_image_size( '-5' ), whose key PHP casts to int -5.

There is precedent for guarding in the same file: image_get_intermediate_size() already has if ( ! $size || ... ) { return false; }, which is the only reason the equivalent ! empty( $imagedata['sizes'][ $size ] ) lookup further down has never hit this.

## Testing Instructions

npm run test:php -- --filter Tests_Image_Size

Two tests are added to tests/phpunit/tests/image/size.php:

  • test_constrain_size_for_editor_additional_image_size() — a registered additional image size still constrains the dimensions, so the guard does not over-restrict.
  • test_constrain_size_for_editor_invalid_size() — a data provider covering null, false, '', 0, 1.5, and an object; each must return the unconstrained dimensions.

Verified on PHP 8.5.9. Reverting only the media.php change and re-running gives three errors, all pointing at the changed line:

1) Tests_Image_Size::test_constrain_size_for_editor_invalid_size with data set "null" (null)
Using null as an array offset is deprecated, use an empty string instead
/var/www/src/wp-includes/media.php:117

2) Tests_Image_Size::test_constrain_size_for_editor_invalid_size with data set "a float" (1.5)
Implicit conversion from float 1.5 to int loses precision
/var/www/src/wp-includes/media.php:117

3) Tests_Image_Size::test_constrain_size_for_editor_invalid_size with data set "an object" (stdClass Object ())
TypeError: Cannot access offset of type stdClass in isset or empty
/var/www/src/wp-includes/media.php:117

With the patch applied, on PHP 8.5.9:

  • --filter Tests_Image_Size — 20 tests, 54 assertions, OK
  • --group media — 862 tests, 2415 assertions, OK (7 skipped)
  • --group image — 284 tests, 680 assertions, OK (6 skipped)

## Note for reviewers

The other six changes in [63177] were reviewed against the same criterion and are safe:

  • class-theme-installer-skin.php — guarded by ! empty()
  • dashboard.php, link-template.phpint blog IDs against int array keys
  • nav-menus.php — explicit (int) casts
  • sitemaps.php — guarded truthy

block-editor.php is the only other one worth a second look: $default_size comes from get_option( 'image_default_size' ), so a corrupt non-scalar option value would now fatal rather than fall back to 'large'. That seems too unlikely to be worth changing, but it is the same class of issue, so flagging it here rather than folding it into this PR.

The general rule for this pattern: swapping in_array( $key, array_keys( $array ), true ) for isset( $array[ $key ] ) is only safe when $key is guaranteed to be a valid array key type.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Investigating the root cause of the Gutenberg CI failure, drafting the patch and the unit tests, drafting this description, and running the test suites quoted above on PHP 8.5.9. The guard condition was changed at my direction. I have reviewed the change and take responsibility for it.

#7 @mukesh27
4 weeks ago

  • Resolution fixed
  • Status closedreopened

@mukesh27 commented on PR #13032:


45 hours ago
#8

@SergeyBiryukov Could you please take a look when you have moments!

Note: See TracTickets for help on using tickets.