fix: avoid PHP 8.5 null array offset deprecation in memoizers#23399
Open
enricobattocchi wants to merge 1 commit into
Open
fix: avoid PHP 8.5 null array offset deprecation in memoizers#23399enricobattocchi wants to merge 1 commit into
enricobattocchi wants to merge 1 commit into
Conversation
Cast the cache key to int in Meta_Tags_Context_Memoizer::get() and Presentation_Memoizer::get() so an unsaved indexable (null id) resolves to a stable integer key instead of triggering the "Using null as an array offset is deprecated" notice on PHP 8.5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 0Coverage decreased (-0.5%) to 53.325%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses a PHP 8.5 deprecation (“Using null as an array offset is deprecated…”) triggered when memoizer cache arrays are accessed with an unsaved Indexable’s null id, primarily affecting editor loads and other fallback-indexable scenarios.
Changes:
- Normalize memoizer cache keys by casting
$indexable->idto an integer before accessing the cache. - Add unit tests covering the null-id memoization path for both memoizers.
- Expose memoizer internal cache in the presentation memoizer test double to enable assertions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/memoizers/presentation-memoizer.php |
Normalizes the cache key in get() to avoid null array offset deprecations. |
src/memoizers/meta-tags-context-memoizer.php |
Normalizes the cache key in get() to avoid null array offset deprecations. |
tests/Unit/Memoizers/Presentation_Memoizer_Test.php |
Adds unit coverage for caching behavior when the Indexable id is null. |
tests/Unit/Memoizers/Meta_Tags_Context_Memoizer_Test.php |
Adds unit coverage for caching behavior when the Indexable id is null. |
tests/Unit/Doubles/Memoizers/Presentation_Memoizer_Double.php |
Adds get_cache() helper for inspecting memoizer cache during tests. |
Comments suppressed due to low confidence (2)
src/memoizers/presentation-memoizer.php:73
get()now normalizes the cache key with(int) $indexable->id, butclear()still unsets using the raw$indexable->id. If an Indexable has anullid, this can still trigger the same "null array offset" deprecation, and it also won’t clear the cache entry that was stored under key0byget(). Consider normalizing the key inclear()as well (for theIndexablebranch).
return $this->cache[ $cache_key ];
}
/**
* Clears the memoization of either a specific indexable or all indexables.
src/memoizers/meta-tags-context-memoizer.php:164
get()now memoizes under(int) $indexable->id, butclear()still unsets using the raw$indexable->idand passes it toPresentation_Memoizer::clear(). If the Indexable id isnull, this can still trigger the PHP 8.5 deprecation and (more importantly)Presentation_Memoizer::clear( null )currently clears the entire presentation cache, which is likely unintended. Normalizing the id to the same integer key inclear()avoids both issues.
return $this->cache[ $cache_key ];
}
/**
* Clears the memoization of either a specific indexable or all indexables.
Comment on lines
+24
to
+28
| /** | ||
| * Used to retrieve the internal cache for testing purposes. | ||
| * | ||
| * @return array<int|string, mixed> The cache. | ||
| */ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
PHP 8.5 introduced a new deprecation,
Using null as an array offset is deprecated, use an empty string instead. Yoast's meta memoizers cache their results in a plain array keyed by the indexable's id. When that id isnull, the array access triggers the deprecation on every call. An indexable id isnullfor any indexable that was built but never saved: the fallback indexable thatIndexable_Repository::for_current_page()returns (for example while the post editor is loading), and, on non-production sites, every indexable (because indexables are only persisted in production). The result isdebug.logbeing flooded with deprecation notices when editing a post, as reported in #23397.Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
nullkey comes from a variable, not a literal (a literal$array[null]is folded to$array['']at compile time). That is why a quick literal-key test looks clean and the issue was initially hard to reproduce.Meta_Tags_Context_Memoizer::get()andPresentation_Memoizer::get(), which key their cache array by$indexable->id.$indexable->idisnullfor unsaved indexables.(int) $indexable->id. A real id is already an integer and is unchanged; anullid becomes0. Real indexable ids start at 1, so0never collides with a real one, and0is a valid array key that does not trigger the deprecation. Behaviour is otherwise identical to before.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
wp-config.php, turn on debug logging by setting bothWP_DEBUGandWP_DEBUG_LOGtotrue.wp-content/debug.log. You will see new lines readingPHP Deprecated: Using null as an array offset is deprecatedthat point atmeta-tags-context-memoizer.phpandpresentation-memoizer.php.composer test -- --filter=Memoizer.Relevant test scenarios
The memoizers serve every indexable type, so it is worth confirming the log stays clean for a post, a page, and a term while viewing them and while editing a post.
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.[yoast-doc-extension], added test instructions for Yoast SEO for Google Docs and attached theGoogle Docs Add-onlabel to this PR.Documentation
Quality assurance
grunt build:imagesand committed the results, if my PR introduces or edits images or SVGs.Innovation
innovationlabel.Fixes #23397