Skip to content

Code Quality: Resolve the isset.variable PHPStan errors - #13023

Open
westonruter wants to merge 8 commits into
WordPress:trunkfrom
westonruter:fix/isset.variable
Open

Code Quality: Resolve the isset.variable PHPStan errors#13023
westonruter wants to merge 8 commits into
WordPress:trunkfrom
westonruter:fix/isset.variable

Conversation

@westonruter

@westonruter westonruter commented Aug 12, 2026

Copy link
Copy Markdown
Member

Empties and deletes the isset.variable PHPStan baseline. All six entries are fixed, so tests/phpstan/baselines/isset.variable.neon is removed along with its line in the includes of phpstan.neon.dist — the intended end state for each of these files.

Every error was the same shape: a call to isset() on a variable that PHPStan can prove is always defined and not nullable, or is never defined at all. In each case the check was dead, and in each case the surrounding code already told you why.

The six errors

File Error Fix
class-custom-image-header.php $_POST always exists elseif ( isset( $_POST ) )else. The superglobal is always set, so the branch was unconditional.
class-wp-oembed.php $loader always exists Dropped && isset( $loader ). PHP_VERSION_ID is constant within a request, so the identical PHP_VERSION_ID < 80000 guard above already decides it. $loader = null is initialized for the benefit of editors that do not correlate the two constant conditions.
media.php $_POST always exists isset( $_POST ) && count( $_POST )! empty( $_POST ), which is that expression by definition and also survives a non-countable $_POST instead of throwing.
class-wp-block-parser.php $namespace always exists Dropped isset( $namespace ) &&. namespace is a non-trailing optional group under PREG_OFFSET_CAPTURE, so PHP always populates it as array( '', -1 ); the -1 !== $namespace[1] test was carrying all the logic.
file.php $stylesheet always exists isset( $stylesheet )$stylesheet, plus a $stylesheet = null in the initializer block above. The sole assignment is guarded by ! empty( $args['theme'] ), so the variable is either null or guaranteed truthy — isset() and truthiness cannot diverge.
template.php $s never defined See below.

load_template() and the $s global

This one was not a redundant check but the opposite: $s is genuinely undefined as far as PHPStan is concerned, because it arrives via extract( $wp_query->query_vars, EXTR_SKIP ). Trunk suppresses the resulting variable.undefined with an inline @phpstan-ignore, and the isset.variable report was baselined.

Two changes let both go. The array is first assigned to a local, since extract() on a property expression gives PHPStan nothing to work with. That local then carries an annotation for the one key the function reads:

/** @var array{ s?: scalar, ... } $query_vars */
$query_vars = $wp_query->query_vars;

An earlier revision of this branch put that shape on WP_Query::$query_vars itself. That was wrong twice over, and 99f51e4 reverts it:

  • It was inaccurate. parse_query() gates s only with is_scalar(), so ints, floats and bools pass through untouched. Tests_Query_ParseQuery::test_parse_query_s_type asserts exactly that — 3, 3.5 and true all survive a round trip unchanged. Hence scalar, not string, and hence the cast that esc_attr() now receives (behavior-preserving, since it already coerces).
  • It cost more than it saved. An unsealed array shape is stricter than a plain array for offset reads: every key other than the one named becomes "might not exist". Narrowing the shared public property removed 4 errors at rule level 10 and introduced 12, across WP_Query itself, WP_Media_List_Table and three REST controllers, plus a further 12 in the parseQuery test file. None of those files changed, so a diff-of-changed-lines check could not have caught it.

Scoped to the local, the same annotation measures at 0 new errors and 3 removed against a full level 10 run. A plain @var is used rather than @phpstan-var so editors read it too. Documenting the full shape of query_vars belongs with #60745, not here.

Brought forward from #11151

#11151 bumped the rule level to 1 and created these baselines. An earlier revision of that branch also fixed level 1 errors across ten files; those fixes were reverted in cce3ac0 so that the pull request stayed limited to the level bump and its tooling, with the fixes to be proposed separately. This is that follow-up for the isset.variable subset.

Two of the six overlap:

Change Status
class-custom-image-header.php Cherry-picked unchanged. 2a3f13d is 95ddce2 from that branch; the two have an identical git patch-id.
file.php Same error, narrower fix. 25de3ac collapsed the chain to if ( $plugin ) … else …, deleting the else { $url = admin_url(); } fallback as unreachable. That reasoning holds — the function returns missing_theme_or_plugin when neither is set — but this PR keeps the fallback and tests elseif ( $stylesheet ) instead, so no reachable-looking branch is removed on the strength of a static-analysis argument. The $stylesheet = null initializer is common to both.

The other four are new. The locate_template() change from that branch is not included here; it addressed variable.undefined and remains deferred.

Follow-up revisions

Each maps to a specific hunk:

Revision Why
r28407 Eliminate use of extract() in get_media_item() — introduced the isset( $_POST ) && count( $_POST ) guard being replaced.
r32298 Escape the $s global — introduced the isset( $s ) / esc_attr( $s ) pair in load_template().
r41721 Introduce sandboxed live editing of PHP files — introduced wp_edit_theme_plugin_file() and its isset( $stylesheet ) check.
r48789 Only call libxml_disable_entity_loader() in PHP < 8 — introduced the PHP_VERSION_ID < 80000 && isset( $loader ) condition.
r60351 Remove unnecessary isset() check in Custom_Image_Header::step_2() — the direct precedent, in the same method, from the 6.9 round of this work.
r61504 Restore block parser in Core — the current WP_Block_Parser::next_token() body.
r61699 Integrate PHPStan into the core development workflow — added the inline @phpstan-ignore variable.undefined in load_template() that this removes.
r63019 Raise the PHPStan rule level to 1 — created the isset.variable baseline this empties.

Draft SVN commit message

Code Quality: Resolve the `isset.variable` PHPStan errors.

Each of the six baselined errors was an `isset()` on a variable that is always defined and not nullable, or never defined at all. The `$_POST` superglobal is always set, so the checks on it in `Custom_Image_Header::step_2()` and `get_media_item()` were unconditional; the latter becomes `! empty( $_POST )`, which is what `isset()` plus `count()` already meant. `PHP_VERSION_ID` is constant within a request, so the guard above the `$loader` check in `WP_oEmbed::_parse_xml()` already decides it. The `namespace` group in `WP_Block_Parser::next_token()` is a non-trailing optional group under `PREG_OFFSET_CAPTURE`, which PHP always populates. In `wp_edit_theme_plugin_file()` the only assignment to `$stylesheet` is guarded by a `! empty()` on the argument it comes from, so a truthiness test cannot diverge from `isset()`.

The remaining error is the reverse case: `$s` in `load_template()` is undefined to static analysis because it arrives through `extract()`. Assigning the query vars to a local first, and annotating that local as `array{ s?: scalar, ... }`, resolves both that report and the inline `@phpstan-ignore` above it. The annotation is deliberately local rather than on `WP_Query::$query_vars`, where an unsealed shape makes every unnamed key "might not exist" and reports more than it resolves. `scalar` rather than `string` is what `WP_Query::parse_query()` actually guarantees, since it gates the value only with `is_scalar()`.

With the last entry fixed, the baseline file and its entry in the `includes` of `phpstan.neon.dist` are removed.

Developed in https://github.com/WordPress/wordpress-develop/pull/13023.
Follow-up to r28407, r32298, r41721, r48789, r60351, r61504, r61699, r63019.

See #65817.

The Props line is deliberately absent — it should come from props-bot once this has been reviewed and tested.

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

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Analysis of each PHPStan error and its surrounding history, the fixes, the commit messages, this description and the draft commit message above. Every change was directed, reviewed and revised by me. The equivalence argument for each fix, the measured error counts, and the reverted WP_Query annotation were all verified in the working tree.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

westonruter and others added 8 commits August 12, 2026 13:31
The `@phpstan-var array{ s?: string, ... }` on the public `WP_Query::$query_vars`
property was both inaccurate and net-negative, so move it to a local annotation
inside the one function that needs it.

It was inaccurate because `parse_query()` gates `s` only with `is_scalar()`, so
ints, floats and bools pass through untouched. That is deliberate, documented
behavior: `Tests_Query_ParseQuery::test_parse_query_s_type` asserts that `3`,
`3.5` and `true` all survive a round trip unchanged.

It was net-negative because an unsealed array shape is stricter than a plain
`array` for offset reads — every key other than `s` becomes "might not exist".
Narrowing the shared property removed 4 errors and introduced 12 more at rule
level 10, spread across `WP_Query` itself, `WP_Media_List_Table` and three REST
controllers, plus a further 12 in the `parseQuery` test file. None of those files
changed, so a diff-of-changed-lines check could not have caught them.

Annotating the local `$query_vars` instead confines the narrowing to
`load_template()`, where it is the only thing `extract()` has to work from. Typing
`s` as `scalar` rather than `string` is the honest type, which in turn makes the
cast in the `esc_attr()` call necessary; that cast is behavior-preserving, since
`esc_attr()` already coerces its argument.

A plain `@var` tag is used rather than `@phpstan-var` so that IDEs read it too.
Documenting the full shape of `query_vars` is left to Core-60745.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props westonruter, irozum.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

irozum

This comment was marked as low quality.

@westonruter

Copy link
Copy Markdown
Member Author

@irozum Hi. It seems like #13023 (review) was written by AI. When you use AI to add reviews, please disclose how you have done so. Otherwise, it is misleading given that your comment says “I” and “me” when actually it was “it”. Please refer to the AI Guidelines.

Please also add any necessary AI disclosure to #12975 (review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants