Code Quality: Annotate three more impure functions for PHPStan - #13077
Code Quality: Annotate three more impure functions for PHPStan#13077tstokes8040 wants to merge 2 commits into
Conversation
PHPStan infers `current_theme_supports()`, `wp_installing()` and `is_front_page()` as pure, so it remembers what each returned and reuses that value at a later call in the same scope. All three read state that can change between calls: `current_theme_supports()` reads the `$_wp_theme_features` global and applies the `current_theme_supports-{$feature}` filter, `wp_installing()` reads a static that the same function can also set, and `is_front_page()` reads the `$wp_query` global.
This resolves three baselined errors: the `! current_theme_supports( 'custom-header', 'flex-height' )` check in `Custom_Image_Header::step_1()`, the `! wp_installing()` branch in `get_transient()`, and the repeated `is_front_page()` call in `redirect_canonical()`. The baselines were regenerated with:
{{{
composer phpstan:baselines -- --identifier=booleanNot.alwaysTrue,booleanAnd.leftAlwaysTrue
}}}
Props tstokes8040.
See #65817.
|
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. |
| * @return bool True if WP is installing, otherwise false. When a `$is_installing` is passed, the function will | ||
| * report whether WP was in installing mode prior to the change to `$is_installing`. | ||
| * | ||
| * @phpstan-impure |
There was a problem hiding this comment.
@westonruter this isn’t related to this PR, but just like with WPCS “ignore” comments I find these to be a bit distracting in the code. it’s like we’re polluting our code to cover the deficiencies of a tool we happen to be using right now.
can these functions and methods not be defined as impure in external config files for PHPStan? if we could do that, it would keep the code clean while making up for the fact that PHPStan didn’t infer the purity aspect properly.
2 participants
PHPStan infers
current_theme_supports(),wp_installing()andis_front_page()as pure, so it remembers what each returned and reuses that value at a later call in the same scope. All three read state that can change between calls:current_theme_supports()$_wp_theme_featuresglobal, and applies thecurrent_theme_supports-{$feature}filter before returningwp_installing()static $installing, which the same function sets when passed an argumentis_front_page()$wp_queryglobalThis resolves three baselined errors:
Custom_Image_Header::step_1(),class-custom-image-header.php:619booleanNot.alwaysTrueget_transient(),option.php:1459booleanNot.alwaysTrueredirect_canonical(),canonical.php:688booleanAnd.leftAlwaysTrueIn the first, the enclosing
elseifchain has already testedcurrent_theme_supports( 'custom-header', 'flex-height' ), so PHPStan carried that answer into the inner check. In the second,get_transient()reaches theelsebranch only whenwp_using_ext_object_cache() || wp_installing()was false, so the later! wp_installing()looked settled.booleanNot.alwaysTruegoes from 8 to 6 andbooleanAnd.leftAlwaysTruefrom 5 to 4. The baselines are generated, not hand-edited; neither reaches zero, so no baseline is deleted andphpstan.neon.distis unchanged.A note on
redirect_canonical()The condition reads
( ! is_front_page() || is_front_page() && get_query_var( 'paged' ) > 1 ). Both calls sit inside one expression with nothing in between, so! A || A && Bcould equally be simplified to! A || Brather than annotated. I chose the annotation because it is independently true —is_front_page()really does read mutable global state, and PHPStan should not assume otherwise anywhere else either — and because rewriting a condition in the canonical redirect path carries behaviour risk for no analytical gain. Happy to switch to the simplification if reviewers prefer it.Testing instructions
npm run typecheck:phpontrunkreports[OK] No errors, because the occurrences are baselined.npm run typecheck:phpreports[OK] No errorsacross 1289 files with both baselines regenerated.composer lintreports no errors for the three modified files.src/wp-includes/query.phpcarries four pre-existingWordPress.DB.PreparedSQL.NotPreparedwarnings at lines 1228 and 1231; the same four are present in the file ontrunkand are untouched by a docblock change at line 462.Documentation-only change; no runtime behaviour is affected.
Related: #13074 annotates
current_user_can(),WP_Filesystem_Base::chmod()andWP_HTML_Processor::next_token()for the same reason, and #13057 annotatesWP_HTML_Tag_Processor::parse_next_tag(). This PR is independent of both and touches disjoint files.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: grouping the remaining boolean PHPStan baselines by root cause, identifying this cluster, measuring the before/after error diff across the whole codebase, regenerating the baselines, and drafting this description. The change itself and the verification runs were reviewed and confirmed by me in a local development environment.
Count of removed errors: 2