chore: PHPStan level 5 in CI + dead-code cleanup (sprint 1)#22
Merged
Conversation
Sprint 1 of the post-audit cleanup, mirroring gds-assistant#33. Started from 97 PHPStan errors at level 5 and went to zero. PHPStan stack (level 5, runs over `src/`): - phpstan/phpstan ^2.2 - szepeviktor/phpstan-wordpress ^2.0 - php-stubs/wp-cli-stubs ^2.12 The bulk of the noise was third-party functions / classes only reached behind `function_exists` / `class_exists` guards (Polylang, Polylang Pro, ACF, Stream, Safe Redirect Manager, Redirection, WooCommerce, Sage CacheTags). All of those moved into `phpstan.neon`'s `ignoreErrors` block with one regex per integration so the analyser doesn't demand stubs for runtime-gated code. Real fixes: - `RestDelegation` + the 4 ability classes that duplicated it all read `$typeObject->rest_namespace ?? 'wp/v2'`, but the property is typed `bool|string` (never null) — switched to `?:` so the fallback covers the empty string / false case the `??` was trying to. - `WP_Post_Type::$rest_namespace ?? ''` patterns folded down the same way across `GenericPostTypeAbility`, `GenericTaxonomyAbility`, `PostTypeAbility`, `TaxonomyAbility` — 14 errors gone with one conceptual fix. - `PostTypeAbility::$postType` and `TaxonomyAbility::$taxonomy` were unused promoted constructor args — removed (plus the matching `new self(...)` call sites in `registerAll()`). - `Plugin::getInstance()` now returns `self` instead of unsafe `new static()` on a non-final class. - `GetBlockAbility`: `$blockType->styles ?? []` was dead; `styles` is always an array. - `PatchBlockAbility`: removed the stale `! empty($blockType->render)` half of the dynamic-block guard (the property doesn't exist; the modern API is `render_callback`). Switched the guard itself to `is_callable($blockType->render_callback)` which matches the runtime-honest check WP_Block_Type's stub doesn't let PHPStan see. - `TranslationAuditAbility`: dropped a redundant `count > 0` after an `empty()` check on the same array. - `RestoreSnapshot::deleteTerm()` and `bulk()` had `WP_Error` in their return types but never returned one — tightened to `array` so the type matches reality. The handful of remaining stub imprecisions (PHPStan can't see past WP's `render_callback: callable`, treating it as always-callable and the unreachable branches that follow as dead code) sit in `phpstan.neon` `ignoreErrors` with paths scoped to the affected file + a comment explaining the stub gap, so they don't mask other findings. CI: - New `composer stan` script. - `lint` job in `.github/workflows/test.yml` now also runs `composer stan` so PHPStan lands alongside Pint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sprint 2 of the post-audit cleanup. Two targeted security fixes. ## XPath DoS guards (WebFetchAbility::applyXPath) The XPath fetch path accepts an LLM-supplied expression and runs it against the fetched DOM. The result-iteration was already bounded (500 nodes, 1 MB), but the `$xpath->query()` call itself is unbounded — pathological expressions (stacked descendant axes like `//.//.//*//.//.//*`) can lock libxml's evaluator for a very long time on small pages. Added two pre-flight checks before invoking the evaluator: - Hard cap on expression length (1000 chars). Real selectors are short — this just deflects the cheap "run a huge expression" form of the attack. - Reject expressions matching `#//\.//.*//\.//#` (two-or-more stacked descendant axes), which is the cheapest known form of XPath amplification. Returns clear WP_Error codes so the model can rewrite the selector. ## Media temp file leak (MediaUploadAbility::sideloadAndRespond) `download_url()` / `wp_tempnam()` create temp files in the URL and base64 upload paths. The previous flow `@unlink`'d on the error branch only, trusting `media_handle_sideload` to move it on success. Any unhandled exception during sideload, metadata write, or the reversible-snapshot wrap-up would leak the temp file. Wrapped the body in try-finally so the unlink runs on every exit path. The `file_exists` check skips the `@`-error noise for the common success path (where sideload already moved the file). PHPStan + Pint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sprint 1 removed the unused $postType arg from PostTypeAbility/ TaxonomyAbility but the tests still passed it. PHP silently dropped the trailing 4th arg and shifted everything: the route became 'post' / 'page' / 'category' instead of '/wp/v2/posts' etc. — so executeRead / executeCreate / executeList all hit rest_no_route 404 in CI. Locally everything passed because the previously-cached ability construction was never re-run; CI starts fresh and exposes the breakage. 16 PHPUnit failures → 0 after this fix (verified via wp-env). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Sprint 1 of the post-audit cleanup, mirroring gds-assistant#33. Foundation pass: type-checker in CI, dead-code removal, real bugs surfaced. No user-visible behaviour change.
Started from 97 PHPStan errors at level 5 and went to zero.
PHPStan level 5 in CI
`phpstan/phpstan ^2.2` + `szepeviktor/phpstan-wordpress ^2.0` + `php-stubs/wp-cli-stubs ^2.12`. `composer stan` script + wired into the lint job.
Most of the noise (~70 errors) was third-party functions / classes only reached behind `function_exists` / `class_exists` guards (Polylang, Polylang Pro, ACF, Stream, Safe Redirect Manager, Redirection, WooCommerce, Sage CacheTags). One regex per integration in `phpstan.neon`'s `ignoreErrors` covers each.
Real fixes
One pattern, 14 errors
`RestDelegation` and the four ability classes that duplicated it all read `$typeObject->rest_namespace ?? 'wp/v2'`, but the property is typed `bool|string` (never null). Switched to `?:` so the fallback covers the empty-string / false case the `??` was trying to handle.
Unused property removal
Other
Pairs with
gds-assistant PR — same sprint, same shape, also lands PHPStan level 5 in CI.