Update PHPStan to v2, improve code quality - #1729
westonruter merged 33 commits into
Conversation
Sorry, something went wrong.
|
Strangely I cannot reproduce the https://github.com/WordPress/performance/actions/runs/12256447140/job/34191719008?pr=1729 error locally right now 🤔 I got it earlier but thought I had resolved it. |
Sorry, something went wrong.
| "phpstan": "phpstan analyse --memory-limit=2048M", | ||
| "phpstan": [ | ||
| "composer --working-dir=phpstan install --no-interaction", | ||
| "phpstan/vendor/bin/phpstan analyse --memory-limit=2048M" |
There was a problem hiding this comment.
So there is a new phpstan root directory?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. Could also be renamed.
Sorry, something went wrong.
There was a problem hiding this comment.
We have an existing tools/phpstan directory that could be reused.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm not sure I wanna mix the config directory with that one
Sorry, something went wrong.
There was a problem hiding this comment.
Since we had build-cs before I could just rename it to that.
Sorry, something went wrong.
This is unfortunate since we previously maintained two separate composer.json files but were able to go to one, as I recall. I wonder if we should bump the minimum PHP version to 7.4 for the entire project to facilitate maintenance and considering that core will likely bump the minimum version to 7.4 in in the middle of 2025 per Core-62622. We could still run PHPUnit on 7.2 in GHA for now if we want to. |
Sorry, something went wrong.
|
Ah true, there was a build-cs folder before, but I can't see where it was removed at first glance. Bumping requirements is of course the easiest route, but I thought we just recently discussed not bumping PHP version requirements unnecessarily if we can avoid it. |
Sorry, something went wrong.
@westonruter is it passing for you locally? |
Sorry, something went wrong.
It was removed when we bumped to PHP 7.2 in #1130. |
Sorry, something went wrong.
Resolve the 16-month-stale conflicts between PR #1729 (PHPStan v2) and trunk: - load.php / hooks.php: keep the PR's move of hooks into hooks.php, and port trunk's in-place changes onto it (standalone plugin list now includes nocache-bfcache and view-transitions, drops web-worker-offloading as experimental; PERFLAB_PLACE_OBJECT_CACHE_DROPIN gating; PHP 7.4 / 4.1.0 header bumps). - composer.json / composer.lock: collapse the PR's separate phpstan/ subfolder back into the root. The subfolder existed only because PHPStan v2 requires PHP 7.4 while the plugin required 7.2; trunk now requires 7.4, so the PHPStan v2 dev-dependencies live directly in the root composer.json. PHPStan is pinned to 2.2.2 with the v2 extension set, plus swissspidy/phpstan-no-private. The phpstan/ subfolder is removed. - phpstan.neon.dist: drop the phpVersion override (inferred from config.platform.php = 7.4), restore root vendor/ paths for php-8-stubs, keep the no.private test ignores, and drop the empty.notAllowed block that trunk already removed. Static-analysis errors surfaced by the v2 upgrade are addressed separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reconcile the ignore configuration with PHPStan v2's stricter rules and renamed identifiers, without suppressing genuine type errors (those are left as follow-up): - Add per-plugin no.private ignores for intra-plugin private API usage surfaced by swissspidy/phpstan-no-private across embed-optimizer, image-prioritizer, speculation-rules, view-transitions and performance-lab, matching the existing dominant-color-images / optimization-detective / webp-uploads / web-worker-offloading entries. - Ignore test-only strictness (phpunit.assertEquals, catch.internalClass) consistent with the existing "excessively strict for tests" ignores. - Update the file-not-found ignore to v2's require.fileNotFound identifier and drop the now-unmatched includeOnce.fileNotFound and test-scoped offsetAccess.invalidOffset / no.private.class entries. - Remove three stale inline @PHPStan-Ignore comments that v2 no longer needs (offsetUnset is now understood to throw; impure return value is remembered correctly). The remaining genuine type errors (argument.type, return.type, etc.) are intentionally left unaddressed in this commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Finally picking this back up... |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 75.55556% with 33 lines in your changes missing coverage. Please review. @@ Coverage Diff @@
## trunk #1729 +/- ##
==========================================
+ Coverage 69.87% 70.03% +0.16%
==========================================
Files 90 91 +1
Lines 7797 7803 +6
==========================================
+ Hits 5448 5465 +17
+ Misses 2349 2338 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
The multisite uninstall routines call `get_sites( array( 'fields' => 'ids', ... ) )`,
which returns an array of integer site IDs at runtime. However,
php-stubs/wordpress-stubs types the conditional return of `get_sites()` with a
*sealed* array shape (`$args is array{fields: 'ids'}`), so the `int[]` narrowing
only applies when `fields => 'ids'` is the sole key. Passing additional query
args (`number`, `update_site_cache`, …) makes the argument no longer a subtype
of the sealed shape, and the return type falls back to `array<int, WP_Site>` —
producing false `argument.type` errors on `switch_to_blog()` and `array_diff()`.
Suppress these with inline `@phpstan-ignore argument.type` directives carrying a
TODO. An `@phpstan-ignore` (rather than an inline `@var`) is used deliberately:
with `reportUnmatchedIgnoredErrors` enabled, the suppression becomes an
unmatched-ignore error once the upstream stub is corrected to use an unsealed
shape (`array{fields: 'ids', ...}`), automatically flagging the workaround for
removal.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. Co-authored-by: swissspidy <swissspidy@git.wordpress.org> Co-authored-by: westonruter <westonruter@git.wordpress.org> To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Sorry, something went wrong.
Summary of changesThis PR was ~16 months stale and conflicting with trunk. Here's everything that was done to get it merge-ready and passing. 1. Merge trunk and resolve conflictsMerged current trunk into the branch and resolved the 7 conflicting files:
2. PHPStan upgraded to v2.2.2, collapsed into the rootThe original PR isolated PHPStan in a separate phpstan/ sub-composer.json because PHPStan v2 needs PHP 7.4 while the plugin still supported 7.2. Since trunk now requires PHP 7.4, that rationale is gone, so the PHPStan v2 dev-dependencies were moved back into the root composer.json (phpstan/phpstan pinned to 2.2.2, plus the v2 extension set and swissspidy/phpstan-no-private), the phpstan/ subfolder was removed, and the lock file regenerated. 3. Drove PHPStan from 181 errors → 0After the merge + v2 upgrade, PHPStan reported 181 errors (16 months of new code only ever checked under v1, now analyzed by the stricter v2 + extensions). These were resolved across focused commits:
Two latent bugs were uncovered and fixed in the process:
4. Over-strict WordPress core stubsThree errors are not bugs in our code — they're cases where php-stubs/wordpress-stubs is stricter than WordPress's documented contract:
These are suppressed with inline @phpstan-ignore directives carrying a TODO to fix the stubs upstream. With reportUnmatchedIgnoredErrors enabled, they self-flag for removal once the upstream stubs are loosened. 5. Validation
Follow-upWorth filing upstream PRs against php-stubs/wordpress-stubs for the three stubs above; the inline ignores will then surface as unmatched and can be removed. |
Sorry, something went wrong.
The PHPCS doc-comment sniff cannot parse the unsealed array-shape marker (...) inside a plain @PARAM tag — it fails to find the parameter name, producing "Missing parameter name" / "Doc comment for parameter missing" errors. Move the unsealed array shapes to @phpstan-param tags (which PHPCS ignores and PHPStan reads), keeping a simpler PHPCS-friendly @PARAM for documentation, in the functions where unsealed @PARAM shapes were introduced: webp_uploads_should_discard_additional_image_file(), image_prioritizer_add_root_schema_properties(), embed_optimizer_add_element_item_schema_properties(), od_add_rest_api_availability_test(), perflab_aea_add_enqueued_assets_test(). Also realign the perflab_get_plugin_availability() @PARAM block after the experimental?: bool addition lengthened the type column (phpcbf). PHPStan remains at zero errors and PHPCS passes on all tracked files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the repo’s static analysis tooling to PHPStan v2 (plus related extensions, including swissspidy/phpstan-no-private) and makes a set of targeted code/docblock adjustments across plugins so the codebase type-checks cleanly under the stricter analyzer while preserving runtime behavior.
Changes:
- Upgrades PHPStan + extensions (incl. phpstan-wordpress v2, phpstan-phpunit v2, phpstan-strict-rules v2) and adds swissspidy/phpstan-no-private.
- Refines PHPStan configuration/stubs and adjusts various docblocks/array-shapes and a few code patterns to satisfy PHPStan v2.
- Refactors Performance Lab’s hook callbacks out of load.php into a dedicated hooks.php.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 7 comments.
Show a summary per file| File | Description |
|---|---|
| tools/phpstan/filtered-functions.stub | Loosens array-shapes to allow additional keys for improved stub compatibility. |
| tools/phpstan/constants.php | Adjusts PHPStan bootstrap constants used to model plugin runtime constants. |
| plugins/webp-uploads/uninstall.php | Adds targeted PHPStan ignore for switch_to_blog() arg typing. |
| plugins/webp-uploads/tests/test-image-edit.php | Removes a PHPStan ignore workaround no longer needed under v2 behavior. |
| plugins/webp-uploads/rest-api.php | Unsets by-ref loop variables to avoid accidental reference leakage. |
| plugins/webp-uploads/image-edit.php | Updates PHPStan array-shapes to allow extra keys / new sources structure. |
| plugins/webp-uploads/hooks.php | Adds PHPStan ignore for pseudo-private WP core function usage + null guard logic. |
| plugins/webp-uploads/helper.php | Tightens return shape for filtered image data; adjusts docblocks. |
| plugins/web-worker-offloading/helper.php | Casts wp_json_encode() result to satisfy stricter typing. |
| plugins/view-transitions/uninstall.php | Adds targeted PHPStan ignore for switch_to_blog() arg typing. |
| plugins/view-transitions/includes/theme.php | Casts wp_json_encode() result to satisfy stricter typing. |
| plugins/view-transitions/includes/settings.php | Adds targeted PHPStan ignore for add_settings_field() args typing. |
| plugins/speculation-rules/uninstall.php | Adds targeted PHPStan ignore for switch_to_blog() arg typing. |
| plugins/speculation-rules/settings.php | Preserves sealed array-shape by returning explicit array literal; adds targeted ignore. |
| plugins/performance-lab/load.php | Loads new hooks.php and removes hook callback definitions from load.php. |
| plugins/performance-lab/includes/site-health/audit-enqueued-assets/hooks.php | Refines docblocks/return shapes for PHPStan v2. |
| plugins/performance-lab/includes/site-health/audit-enqueued-assets/helper.php | Refines array-shapes (allow extra keys) and corrects return shape docblock. |
| plugins/performance-lab/includes/site-health/audit-autoloaded-options/helper.php | Casts size_format() result to string for stricter typing. |
| plugins/performance-lab/includes/admin/plugins.php | Adds targeted PHPStan ignore for plugins_api() args typing; refines docblocks. |
| plugins/performance-lab/hooks.php | New file: extracted Performance Lab hook callbacks from load.php. |
| plugins/optimization-detective/uninstall.php | Adds targeted PHPStan ignores for array_diff() + switch_to_blog() typing. |
| plugins/optimization-detective/tests/test-class-od-element.php | Removes PHPStan ignores around exception flow. |
| plugins/optimization-detective/site-health.php | Refines docblocks/array-shapes for PHPStan v2. |
| plugins/optimization-detective/optimization.php | Casts wp_json_encode() result to satisfy stricter typing. |
| plugins/optimization-detective/helper.php | Replaces wp_array_slice_assoc() usage with a PHPStan-friendlier key intersection. |
| plugins/optimization-detective/class-od-url-metric.php | Allows extra keys in schema-related array-shape docblocks. |
| plugins/optimization-detective/class-od-url-metric-group.php | Avoids offset access on possibly-null key result to satisfy PHPStan. |
| plugins/optimization-detective/class-od-html-tag-processor.php | Adds targeted ignore for pseudo-private WP class usage. |
| plugins/image-prioritizer/helper.php | Refines docblocks/array-shapes for PHPStan v2. |
| plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php | Tightens docblock return semantics for a method that always returns false. |
| plugins/embed-optimizer/tests/test-hooks.php | Updates data provider return shape for added optional value. |
| plugins/embed-optimizer/helper.php | Refines docblocks/array-shapes for PHPStan v2. |
| plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php | Adjusts types/casts and CSS escaping edge-case for stricter analysis. |
| plugins/dominant-color-images/hooks.php | Refines docblocks/array-shapes to allow extra keys and align with PHPStan v2. |
| phpstan.neon.dist | Updates ignore rules and stubs for PHPStan v2 and phpstan-no-private integration. |
| composer.lock | Locks updated PHPStan ecosystem versions and adds phpstan-no-private. |
| composer.json | Updates dev dependencies to PHPStan v2-compatible versions and adds phpstan-no-private. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
|
We're in the green!! I'll admit I was surprised when I realized this plugin was still on v1. @swissspidy Thank you for getting this started so long ago. |
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Following up on Copilot's review feedback that the require/requireOnce fileNotFound ignores were unscoped (and would hide real missing-require regressions): give TESTS_PLUGIN_DIR a distinctive, non-resolvable sentinel value and scope those ignores to it via a message regex. TESTS_PLUGIN_DIR is set to the plugin-under-test directory at runtime, so it varies per suite and cannot be modeled statically; its require() targets (uninstall.php and conditionally-loaded files) are unavoidable false positives. TESTS_REPO_ROOT_DIR is left as './' because it correctly resolves to the repo root (PHPStan's working directory), so its requires resolve and those files are actually analyzed. This keeps real require regressions reported everywhere except the known TESTS_PLUGIN_DIR false-positive pattern. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Following up on Copilot's review: webp_uploads_generate_additional_image_source() calls wp_filesize( $image['path'] ), but $image comes from the pluggable webp_uploads_pre_generate_additional_image_source filter, so a third party could return a non-string `path`, causing a TypeError (wp_filesize() requires a string). Restore the is_string( $image['path'] ) runtime guard, and type the filter's `path` as non-empty-string|mixed so the untrusted value makes the check meaningful to PHPStan (rather than being flagged as alreadyNarrowedType) while still documenting that it is normally a non-empty path. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| 0x002D === $code_unit | ||
| ) { | ||
| $result .= '\\' . $ident[ $i ]; | ||
| $result .= '\\-'; |
There was a problem hiding this comment.
Note from Claude about this change:
Now I can explain all three parts precisely.
What the error means
Offset *NEVER* does not exist on non-empty-string.
$ident[$i] is a string-offset access. The *NEVER* is the type PHPStan inferred for the offset expression $i at that line — never (its "bottom"/impossible type). So PHPStan is really saying: "by the time control reaches line 259, $i has type never, i.e. I believe this line is unreachable — and you can't index a string with an impossible offset." When the offset is never, it phrases it as "offset NEVER does not exist."
(The "on non-empty-string" part is just because, inside the for loop, the loop can only run when strlen($ident) >= 1, so PHPStan has narrowed $ident from string to non-empty-string.)
Why it's never at 259 but fine at 280
The difference is the guard around 259:
if ( 0 === $i && 1 === $length && 0x002D === $code_unit ) {
$result .= '\\' . $ident[ $i ]; // 259Those equality constraints (0 === $i and 1 === $length) feed PHPStan's loop/offset analysis, and under bleedingEdge it wrongly concludes this exact combination can't occur — collapsing $i to never. It's a false positive: escape_css("-") (a single hyphen) genuinely reaches line 259 at runtime — trace it: $length=1, $i=0, $code_unit=ord('-')=0x002D, it skips the earlier ifs, and hits this branch.
Line 280 has no such narrowing guard — it's the loop's plain "otherwise" fall-through, where $i is just the normal int<0, max> loop index, which is a valid offset on the string. So no error there. Same text, different inferred type for $i.
Why I replaced it with '\\-'
The branch is guarded by 0x002D === $code_unit, and $code_unit = ord( $ident[$i] ) — so inside this branch $ident[$i] is provably the character - (chr(0x2D)). Therefore:
'\\' . $ident[ $i ] // is always exactly '\-'
'\\-' // identical outputReplacing it with the literal:
- produces byte-for-byte identical output, and
- removes the $ident[$i] access that PHPStan was (incorrectly) flagging — without an @phpstan-ignore suppression.
Line 280 keeps $ident[$i] because there the character is arbitrary (could be anything), so the literal substitution wouldn't apply.
I'll be honest: I can't fully reverse-engineer PHPStan's exact internal step that produces the never here — it's a known-ish quirk of its string-offset reasoning inside loops with equality-narrowed indices. The substitution is a clean, behavior-preserving way around it.
If you'd prefer to keep the code symmetric with line 280 instead, the alternative is to restore '\\' . $ident[ $i ] and add an // @phpstan-ignore offsetAccess.notFound (false positive: $i is 0 for a single "-") — but I lean toward the literal since it's provably equivalent and arguably clearer. Want me to switch to the ignore-based version?
Sorry, something went wrong.
… map
The most-common-LCP-element computation used three arrays kept in sync by hand
($seen_breadcrumbs, $breadcrumb_counts, $breadcrumb_element). PHPStan cannot
correlate keys across separate arrays, so taking key() from one and indexing
another produced offsetAccess.invalidOffset / offsetAccess.notFound and needed
a defensive isset()/null guard.
Combine them into a single $breadcrumbs map keyed by element XPath holding
{count, element}. This removes the out-of-sync risk, replaces the O(n)
array_search() lookup with an O(1) keyed isset(), and lets reset() on the
count-guarded non-empty array return the record directly, so no isset()/null
dance is needed. Behavior is unchanged: last-seen element per breadcrumb, most
frequent wins.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…te_additional_image_source() Covers the branch where the webp_uploads_pre_generate_additional_image_source filter short-circuits with a file and path but no filesize, in which case the filesize is derived via wp_filesize() from the filter-provided path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…_editor_output_format() Covers the two leading conditionals of the previously-untested filter: a non-array output format is normalized to an empty array, and an existing mapping is returned unchanged when the source mime type is null. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Avoid setting 'element' twice on a breadcrumb's first occurrence: initialize it with count 1 (and the element) when first seen, and only increment the count and override the element on subsequent occurrences. Since the count now starts at 1 and only ever increments, type it as int<1, max>. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Fixes #776
Relevant technical choices