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)
This ticket was mentioned in PR #12929 on WordPress/wordpress-develop by @mukesh27.
5 weeks ago
#2
- Keywords has-patch added; needs-patch removed
#3
follow-up:
↓ 4
@
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
@
5 weeks ago
- Milestone Future Release → 7.2
Replying to siliconforks:
Note that in some cases
issetmight not give the same result asin_array. For example, if the value of the array element isnull,issetwill returnfalse.
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.
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 coveringnull,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.php—intblog IDs againstintarray keysnav-menus.php— explicit(int)castssitemaps.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.
@mukesh27 commented on PR #13032:
45 hours ago
#8
@SergeyBiryukov Could you please take a look when you have moments!
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
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.