Code Quality: Resolve the isset.variable PHPStan errors - #13023
Code Quality: Resolve the isset.variable PHPStan errors#13023westonruter wants to merge 8 commits into
Conversation
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>
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
@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). |
Empties and deletes the
isset.variablePHPStan baseline. All six entries are fixed, sotests/phpstan/baselines/isset.variable.neonis removed along with its line in theincludesofphpstan.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
class-custom-image-header.php$_POSTalways existselseif ( isset( $_POST ) )→else. The superglobal is always set, so the branch was unconditional.class-wp-oembed.php$loaderalways exists&& isset( $loader ).PHP_VERSION_IDis constant within a request, so the identicalPHP_VERSION_ID < 80000guard above already decides it.$loader = nullis initialized for the benefit of editors that do not correlate the two constant conditions.media.php$_POSTalways existsisset( $_POST ) && count( $_POST )→! empty( $_POST ), which is that expression by definition and also survives a non-countable$_POSTinstead of throwing.class-wp-block-parser.php$namespacealways existsisset( $namespace ) &&.namespaceis a non-trailing optional group underPREG_OFFSET_CAPTURE, so PHP always populates it asarray( '', -1 ); the-1 !== $namespace[1]test was carrying all the logic.file.php$stylesheetalways existsisset( $stylesheet )→$stylesheet, plus a$stylesheet = nullin the initializer block above. The sole assignment is guarded by! empty( $args['theme'] ), so the variable is eithernullor guaranteed truthy —isset()and truthiness cannot diverge.template.php$snever definedload_template()and the$sglobalThis one was not a redundant check but the opposite:
$sis genuinely undefined as far as PHPStan is concerned, because it arrives viaextract( $wp_query->query_vars, EXTR_SKIP ). Trunk suppresses the resultingvariable.undefinedwith an inline@phpstan-ignore, and theisset.variablereport 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:An earlier revision of this branch put that shape on
WP_Query::$query_varsitself. That was wrong twice over, and 99f51e4 reverts it:parse_query()gatessonly withis_scalar(), so ints, floats and bools pass through untouched.Tests_Query_ParseQuery::test_parse_query_s_typeasserts exactly that —3,3.5andtrueall survive a round trip unchanged. Hencescalar, notstring, and hence the cast thatesc_attr()now receives (behavior-preserving, since it already coerces).arrayfor 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, acrossWP_Queryitself,WP_Media_List_Tableand three REST controllers, plus a further 12 in theparseQuerytest 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
@varis used rather than@phpstan-varso editors read it too. Documenting the full shape ofquery_varsbelongs 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.variablesubset.Two of the six overlap:
class-custom-image-header.phpgit patch-id.file.phpif ( $plugin ) … else …, deleting theelse { $url = admin_url(); }fallback as unreachable. That reasoning holds — the function returnsmissing_theme_or_pluginwhen neither is set — but this PR keeps the fallback and testselseif ( $stylesheet )instead, so no reachable-looking branch is removed on the strength of a static-analysis argument. The$stylesheet = nullinitializer is common to both.The other four are new. The
locate_template()change from that branch is not included here; it addressedvariable.undefinedand remains deferred.Follow-up revisions
Each maps to a specific hunk:
Eliminate use of extract() in get_media_item()— introduced theisset( $_POST ) && count( $_POST )guard being replaced.Escape the $s global— introduced theisset( $s )/esc_attr( $s )pair inload_template().Introduce sandboxed live editing of PHP files— introducedwp_edit_theme_plugin_file()and itsisset( $stylesheet )check.Only call libxml_disable_entity_loader() in PHP < 8— introduced thePHP_VERSION_ID < 80000 && isset( $loader )condition.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.Restore block parser in Core— the currentWP_Block_Parser::next_token()body.Integrate PHPStan into the core development workflow— added the inline@phpstan-ignore variable.undefinedinload_template()that this removes.Raise the PHPStan rule level to 1— created theisset.variablebaseline this empties.Draft SVN commit message
The
Propsline 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_Queryannotation 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.