Repository navigation
Code Quality: Resolve the isset.variable PHPStan errors - #13023
westonruter wants to merge 11 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). |
apermo
left a comment
There was a problem hiding this comment.
I have some minor improvements for readability. But other than that looking good to me.
Co-authored-by: Christoph Daum <c.daum@me.com>
Co-authored-by: Christoph Daum <c.daum@me.com>
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 #13023.
Follow-up to r28407, r32298, r41721, r48789, r60351, r61504, r61699, r63019.
Props westonruter, apermo.
See #65817.
git-svn-id: https://develop.svn.wordpress.org/trunk@63351 602fd350-edb4-49c9-b593-d223f7449a82
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 WordPress/wordpress-develop#13023.
Follow-up to r28407, r32298, r41721, r48789, r60351, r61504, r61699, r63019.
Props westonruter, apermo.
See #65817.
Built from https://develop.svn.wordpress.org/trunk@63351
git-svn-id: http://core.svn.wordpress.org/trunk@62544 1a063a9b-81f0-0310-95a4-ce76da25c4cd
* WP-r55827: Docs: Fix a few more typos in DocBlocks and inline comments. Follow-up to https://core.trac.wordpress.org/changeset/6779, https://core.trac.wordpress.org/changeset/10565, https://core.trac.wordpress.org/changeset/12023, https://core.trac.wordpress.org/changeset/25224, https://core.trac.wordpress.org/changeset/27533, https://core.trac.wordpress.org/changeset/32806, https://core.trac.wordpress.org/changeset/34777, https://core.trac.wordpress.org/changeset/45262, https://core.trac.wordpress.org/changeset/46594, https://core.trac.wordpress.org/changeset/55823, https://core.trac.wordpress.org/changeset/55824. WP:Props Presskopp. See https://core.trac.wordpress.org/ticket/57840. --- Merges https://core.trac.wordpress.org/changeset/55827 / WordPress/wordpress-develop@faee35ab0d to ClassicPress. * WP-r55897: Embeds: Add support for TikTok creator profiles. Follow-up to initial support in https://core.trac.wordpress.org/changeset/47216 / https://core.trac.wordpress.org/ticket/49083. WP:Props mukesh27, anveshika, Ankit K Gupta. Fixes https://core.trac.wordpress.org/ticket/55784. --- Merges https://core.trac.wordpress.org/changeset/55897 / WordPress/wordpress-develop@e2a747662e to ClassicPress. * WP-r55914: Coding Standards: Use strict comparison in `wp-includes/class-wp-oembed.php`. Follow-up to https://core.trac.wordpress.org/changeset/13996. WP:Props aristath, poena, afercia, SergeyBiryukov. See https://core.trac.wordpress.org/ticket/57839. --- Merges https://core.trac.wordpress.org/changeset/55914 / WordPress/wordpress-develop@2888dafcab to ClassicPress. * WP-r55961: Embeds: Add Anghami as a trusted oEmbed provider. WP:Props swissspidy, Boniu91, tb1909, monzuralam, azizantoun, audrasjb. Fixes https://core.trac.wordpress.org/ticket/49850 --- Merges https://core.trac.wordpress.org/changeset/55961 / WordPress/wordpress-develop@d3a3f019d6 to ClassicPress. * WP-r56178: Docs: Replace multiple single line comments with multi-line comments. This changeset updates various comments as per WordPress PHP Inline Documentation Standards. See https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/https://core.trac.wordpress.org/ticket/5-inline-comments. Follow-up to https://core.trac.wordpress.org/changeset/56174, https://core.trac.wordpress.org/changeset/56175, https://core.trac.wordpress.org/changeset/56176, https://core.trac.wordpress.org/changeset/56177. WP:Props costdev, audrasjb. See https://core.trac.wordpress.org/ticket/58459. Conflicts: - src/wp-includes/class-wp-image-editor-imagick.php --- Merges https://core.trac.wordpress.org/changeset/56178 / WordPress/wordpress-develop@bf911a96a8 to ClassicPress. * Fix merge conflicts from 56178 * WP-r58091: Embeds: Add Bluesky as a trusted oEmbed provider. WP:Props swissspidy, thelovekesh, peterwilsoncc, bnewboldbsky. Fixes https://core.trac.wordpress.org/ticket/61020. --- Merges https://core.trac.wordpress.org/changeset/58091 / WordPress/wordpress-develop@1911cbabb2 to ClassicPress. * WP-r58362: Embeds: Remove SlideShare embed block variation. The oEmbed endpoint for SlideShare is effectively deprecated and throws an error for the majority of slide-decks using the service. Capitalize the name of the service in the deprecation notice in accordance with the wordmark. WP:Props peterwilsoncc, mukesh27, swissspidy, talldanwp, dd32. Fixes https://core.trac.wordpress.org/ticket/61349. --- Merges https://core.trac.wordpress.org/changeset/58362 / WordPress/wordpress-develop@1ea82dcd26 to ClassicPress. * WP-r59598: Embeds: Add Canva to oEmbed provider allowlist. WP:Props swissspidy. Fixes https://core.trac.wordpress.org/ticket/58840. --- Merges https://core.trac.wordpress.org/changeset/59598 / WordPress/wordpress-develop@c0d073d62d to ClassicPress. * WP-r60307: Docs: Correct parameter type for the `oembed_dataparse` filter. This commit updates the filter documentation to indicate that it can both take and return a `string|false` value. Documentation for functions attached to the filter has been similarly corrected: * `WP_oEmbed::_strip_newlines()` * `wp_filter_oembed_result()` * `wp_filter_oembed_iframe_title_attribute()` Follow-up to https://core.trac.wordpress.org/changeset/12153, https://core.trac.wordpress.org/changeset/25723, https://core.trac.wordpress.org/changeset/34903, https://core.trac.wordpress.org/changeset/34974, https://core.trac.wordpress.org/changeset/44942. WP:Props justlevine. See https://core.trac.wordpress.org/ticket/63268. --- Merges https://core.trac.wordpress.org/changeset/60307 / WordPress/wordpress-develop@8a0f8dfe69 to ClassicPress. * WP-r60345: Embeds: Remove screencast.com from oEmbed allow list. Screencast have updated their URL structure to use a sub-domain and the prior oEmbed endpoint in the allow list now returns a `410 Gone` response. For discussion regarding the legacy endpoint refer to [comment:35:ticket:61941 https://core.trac.wordpress.org/ticket/61941, comment https://core.trac.wordpress.org/ticket/35] onwards. See https://core.trac.wordpress.org/ticket/61941. WP:Props audrasjb, brhodes, dustintechsmith, joedolson, johnbillion, nikunj8866, paulstanos, peterwilsoncc, sandeepdahiya, sergeybiryukov, sirlouen, swissspidy. --- Merges https://core.trac.wordpress.org/changeset/60345 / WordPress/wordpress-develop@15fb5354ec to ClassicPress. * WP-r61797: Embeds: Remove unnecessary `false` check in oEmbed data fetching. Developed in WordPress/wordpress-develop#11126 Follow up to r40628. WP:Props soean, swissspidy, ocean90, sajib1223, manhar. Fixes https://core.trac.wordpress.org/ticket/64784. --- Merges https://core.trac.wordpress.org/changeset/61797 / WordPress/wordpress-develop@c6c1256c57 to ClassicPress. * WP-r62501: Embeds: Validate registered oEmbed providers. A plugin may register an oEmbed provider through the `oembed_providers` filter using a malformed structure, such as an associative array rather than the expected tuple of a provider endpoint URL string at index 0 and an optional boolean regex flag at index 1. This previously produced `Undefined array key` PHP warnings when `WP_oEmbed::get_provider()` destructured the entry. Introduce a private `sanitize_provider()` method that validates the match pattern and provider data, normalizing the optional regex flag to a boolean. The constructor now skips malformed entries and reports each one via `_doing_it_wrong()`, and `get_provider()` likewise ignores any invalid entries it encounters at runtime. Developed in WordPress/wordpress-develop#11568. WP:Props sukhendu2002, westonruter, bradshawtm, rollybueno. Fixes https://core.trac.wordpress.org/ticket/65068. --- Merges https://core.trac.wordpress.org/changeset/62501 / WordPress/wordpress-develop@73ed971672 to ClassicPress. * WP-r63351: 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 WordPress/wordpress-develop#13023. Follow-up to r28407, r32298, r41721, r48789, r60351, r61504, r61699, r63019. WP:Props westonruter, apermo. See https://core.trac.wordpress.org/ticket/65817. Conflicts: - phpstan.neon.dist - src/wp-includes/class-wp-block-parser.php - src/wp-includes/template.php --- Merges https://core.trac.wordpress.org/changeset/63351 / WordPress/wordpress-develop@558d0ff66d to ClassicPress. * Fix conflicts from 63351 * WP-r63605: Docs: Remove Screencast from the list of supported oEmbed providers. This is just a documentation change, as support for screencast.com has already been removed as of WordPress 6.8.2. This commit updates the list of supported providers in the `oembed_providers` filter DocBlock to reflect that change. Developed in WordPress/wordpress-develop#13479. Follow-up to r60345. WP:Props maheshpawar2012, westonruter, SergeyBiryukov. Fixes https://core.trac.wordpress.org/ticket/66090. --- Merges https://core.trac.wordpress.org/changeset/63605 / WordPress/wordpress-develop@98a8a2f0ab to ClassicPress. * WP-r63904: Embeds: Remove someecards.com from the oEmbed allow list. The someecards.com oEmbed endpoint returns a `404 Not Found` response and card pages no longer expose an oEmbed discovery link, so the provider can no longer be used. This removes the `someecards.com` and `some.ly` patterns from the list of trusted providers. Follows https://core.trac.wordpress.org/changeset/60345. WP:Props johnbillion, suryakantupadhyay, adamsilverstein. Fixes https://core.trac.wordpress.org/ticket/66078. --- Merges https://core.trac.wordpress.org/changeset/63904 / WordPress/wordpress-develop@7c2e110d43 to ClassicPress. * WP-r63907: Embeds: Update the ReverbNation oEmbed endpoint. ReverbNation has moved its site and oEmbed API to the `legacy.reverbnation.com` subdomain. The previous `www.reverbnation.com/oembed` endpoint returns a `404 Not Found` response and does not redirect, so embeds of artist and song URLs stopped working. This points the provider at the new endpoint and extends the URL pattern to match `legacy.reverbnation.com`, which `www.reverbnation.com` URLs now redirect to. WP:Props johnbillion, yogeshbhutkar, arkaprabhachowdhury, adamsilverstein. Fixes https://core.trac.wordpress.org/ticket/66079. --- Merges https://core.trac.wordpress.org/changeset/63907 / WordPress/wordpress-develop@66772c8fd7 to ClassicPress. --------- Co-authored-by: Sergey Biryukov <sergeybiryukov@git.wordpress.org> Co-authored-by: Pascal Birchler <swissspidy@git.wordpress.org> Co-authored-by: Jb Audras <audrasjb@git.wordpress.org> Co-authored-by: Peter Wilson <peterwilsoncc@git.wordpress.org> Co-authored-by: Weston Ruter <westonruter@git.wordpress.org> Co-authored-by: Jake Spurlock <whyisjake@git.wordpress.org> CP:Props mattyrob, xxsimoxx
✅ Committed in r63351 (558d0ff)
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.