Uh oh!
There was an error while loading. Please reload this page.
Fix static analysis regressions introduced during the 7.1 cycle - #13064
Open
westonruter wants to merge 11 commits into
Open
Fix static analysis regressions introduced during the 7.1 cycle#13064westonruter wants to merge 11 commits into
westonruter wants to merge 11 commits into
Conversation
r62178 replaced `@return string|void` with `@return string|null` on WP_Widget::form(), on the premise that void cannot be part of a union type. That holds for PHP's native return types, but not for PHPDoc, where `string|void` is the documented way to say a method may return a string or may return nothing at all. PHPStan reads it exactly that way, and treats `string|null` instead as an obligation to return. The base implementation echoes a notice and returns 'noform', while every subclass echoes its own markup and falls off the end. Tightening the declared type therefore put all 18 subclass form() overrides in breach of it, for 20 reported errors. Four of those overrides live in bundled themes, which r62178 did not touch and so could not have updated alongside the parent. form_callback() is unaffected: it initialises $return to null and always returns it, so its own string|null annotation stays accurate, as does the null|string documented for $return on the in_widget_form action. Only the method that may legitimately not return at all needed void back. Regenerating the baselines drops 18 entries covering 20 errors from tests/phpstan/baselines/return.missing.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sixteen call sites reached seven private static methods through `static::` rather than `self::`. Late static binding resolves to the runtime class, but a private method is not inherited, so the two are only equivalent for as long as nothing subclasses WP_Theme_JSON. The moment something does, `static::` resolves against a class where the method is not visible. Nothing in core extends WP_Theme_JSON today, so this is latent rather than an active defect. It is worth correcting regardless: `self::` is what a private method actually means, and the class is a plausible extension point, being the one Gutenberg mirrors. The methods involved are sanitize_viewport_settings(), is_valid_viewport_breakpoint_size(), get_viewport_breakpoint_value_in_pixels(), update_paragraph_text_indent_selector(), update_button_width_declarations(), get_block_name_from_metadata_path(), and get_feature_selector(). All were introduced after 7.0. Regenerating the baselines drops 7 entries covering 14 errors from tests/phpstan/baselines/staticClassAccess.privateMethod.neon. The remaining entries in that file are unrelated to this class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The method has been annotated `@param object` for both `$metadata` and `$node` since 6.3.0, but it has only ever been passed arrays, and only ever treats them as arrays: it reads `$metadata['selectors']` and `$node[ $feature ]`, builds `$node[ $feature ][ $subfeature ]`, and unsets through the reference. Given a real object, the first subscript would be fatal. The wrong type propagated. Because `$node` is taken by reference, callers had their own variable narrowed to object after the call, so the same node then reported the mirror-image error when handed to process_pseudo_selectors(), which correctly documents `array`. Code added since 7.0 in get_styles_for_block() made that visible in three places. Correcting the two annotations resolves all five errors the file reported, without touching a line of executable code, and documents that features promoted to their own selector are removed from `$node`. Regenerating the baselines drops 3 entries covering 5 errors from tests/phpstan/baselines/argument.type.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shape-mismatch guard tested `array() !== $current` after already requiring `! array_is_list( $current )`. Since array_is_list() returns true for an empty array, the negation cannot hold unless $current is non-empty, so the trailing comparison was always true and never decided anything. Only that clause is removed. The matching test on $incoming stays, because it is live: $incoming is a list at that point and may legitimately be empty, which is the documented exemption letting replace() clear a list. So does the similar check further down, where array_is_list( $current ) is asserted rather than negated and an empty array therefore still reaches it. Behaviour is unchanged. The 78 view config tests pass. Regenerating the baselines drops 1 entry from tests/phpstan/baselines/notIdentical.alwaysTrue.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two guards added since 7.0 tested isset() on a declared property that has a non-null default and is therefore never unset: * WP_Posts_List_Table::get_primary_column_aria_label() paired isset( $item->post_title ) with ! empty( $item->post_title ). WP_Post declares $post_title as a string, and empty() already covers unset, null and '', so the isset() decided nothing. * wp_get_block_state_style_rules() paired isset( $block_type->selectors ) with is_array( $block_type->selectors ). WP_Block_Type declares `public $selectors = array()`. Only the isset() is removed in each case. The is_array() test on $selectors stays: the property carries no native type, so a plugin can assign a non-array to it, and that check is doing real work. Behavior is unchanged. The 46 block supports states tests pass. Regenerating the baselines drops 2 entries from tests/phpstan/baselines/isset.property.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The delete-users confirmation block queries $wpdb for a user's posts and links without the file ever importing the global. It works, since a top-level admin page runs at global scope, but nothing said so, and static analysis reported the variable as possibly undefined. Import it with the `@global` docblock core uses elsewhere for the same purpose, matching edit.php and edit-comments.php. The tag alone is not enough: the PHPStan visitor bridging core's `@global` tags acts on `global` statements, so the statement carries the type and the tag documents it. Two of the three reports on the file are on lines added since 7.0; the third predates it and is fixed by the same declaration. Regenerating the baselines drops the users.php entry from tests/phpstan/baselines/variable.undefined.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The HTML API passes null for substr_compare()'s $length, which has always meant "compare the full length". PHP 7.4 spelled the parameter `int $length = null`, where the null default makes it implicitly nullable, and PHP 8.0 only made that explicit as `?int $length = null`. The behavior never differed; running the call on PHP 7.4 confirms null compares the whole string rather than coercing to a length of 0, which is the failure that would matter here. PHPStan reads the two spellings from two sources and only one is right. Its PHP 8 stub carries `?int`, but the pre-8.0 resources/functionMap.php records plain `int`, having dropped the implicit nullability when it was transcribed from the old manual. Since phpVersion.min is 70400, the legacy map wins and null is reported as invalid. Move the entry out of the baseline and into ignoreErrors, where the surrounding comment records all of the above. A baseline entry is a promise to fix something, and there is nothing here to fix: the call is correct on every version WordPress supports, and passing an explicit length purely to satisfy the analyzer would change working code to suit a tooling bug. reportUnmatched is false so the entry lapses quietly once PHPStan corrects its map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are user preferences read straight off a WP_User through the magic __get(), exactly like rich_editing and syntax_highlighting beside them, but neither was listed among the class's @Property tags. Nothing could resolve them, so every read was unverifiable. That produced a misleading report on the profile screen. user-edit.php reads 43 properties off $profile_user, which is WP_User|false because get_user_to_edit() returns get_userdata() unchanged. Access on a union is only reported from level 7, so at level 5 the other 41 reads passed on the strength of their @Property tags, and only these two, which resolved to nothing, fell through to a complaint about the union. The message named the union, but the union was not the cause: adding a guard for the false case merely turned each into "access to an undefined property" on the same line. Documenting the two properties resolves them with no guard at all. The false case is unreachable in any event. The screen already dies with "Invalid user ID." when get_userdata() rejects $user_id, and $user_id is not reassigned between that check and the call, so the user provably exists. PHPStan cannot connect a guard phrased in terms of get_userdata() to a later call to get_user_to_edit(). infinite_scrolling arrived after 7.0 with the Media Library option; comment_shortcuts long predates it and was missing for the same reason. Regenerating the baselines drops 2 entries from tests/phpstan/baselines/property.nonObject.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The method proxies four kinds of value: get_variations() gives array[], get_uses_context() gives string[], a handles property with more than one entry gives string[], and a single entry gives a string, with null for anything it does not recognise. The documented return listed only string|string[]|null, so the array[] from the variations branch was never covered. That branch has been there since 6.5.0, but the omission was invisible while void sat in the union: PHPStan takes void to mean the method may return nothing and does not hold the remaining types to account. r62178 dropped void in favour of null, which turned the union into a contract and surfaced the gap immediately. This is the second such report from that commit. The first was WP_Widget::form(), where the correct fix was to restore void because the subclasses genuinely return nothing. Here void was misleading, since every path returns a value, so the fix is to finish the union rather than reinstate it. Behaviour is unchanged. The 104 block type tests pass. Regenerating the baselines drops 1 entry from tests/phpstan/baselines/return.type.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
r62680 gave term_exists() a conditional return type, and the branch for an empty $taxonomy says int|null. The function returns a string there. Without a taxonomy the query keeps its default 'fields' => 'ids', so the value shifted off the result set is a term ID, and the function hands it back as `return (string) $_term;`. Only when a taxonomy is passed does 'fields' become 'all' and the array of term_id and term_taxonomy_id get returned instead. The prose above the tag says "Returns the term ID", which is silent on int versus string; the conditional type resolved that silence the wrong way. Say numeric-string|null for that branch. The error this clears is the analyzer correctly reporting the function against its own declared type, not a fault in the function. Nothing downstream depended on the incorrect int: a full run before regenerating the baselines reported no new errors anywhere, only the stale entry for this one. The 26 term_exists tests pass. Regenerating the baselines drops 1 entry from tests/phpstan/baselines/return.type.neon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
r62822 typed $comment_ID and $comment_post_ID as numeric-string, which is what hydrating a comment from the database produces. One place in core then contradicts it: get_comment_to_edit() casts both to int and writes them back onto the same object, so from that point on the object no longer matches its own declared type. Widen both to numeric-string|int and say in the description which function does it. The alternative, dropping the casts in get_comment_to_edit(), would change what its callers have received for years, and this is the same hedge already applied to WP_User::$user_level, documented as int|numeric-string|''. The narrower type was also costing precision rather than buying it. Because the analysis runs with treatPhpDocTypesAsCertain disabled, a union carrying int satisfies the many core call sites that hand these properties to parameters typed int, all of which had been baselined as string given. Regenerating drops 39 entries across 9 files, from comment.php and the REST controller to the recent comments widget, and introduces nothing: a full run before regenerating reported no new errors anywhere. The 794 comment tests pass. 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. |
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 freeto 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.
Fixes the static analysis regressions introduced during the 7.1 cycle: errors that trunk reports and a baseline generated from 7.0.0's
srcdoes not.Each was verified before being touched. An error is only treated as a regression if the symbol whose type provoked it was itself changed after the 7.0 tag — following the symbol rather than the reporting line, because a docblock edit in one file surfaces errors in files that have not been edited in years. r62178 changing
WP_Widget::form()produced 20 errors in widget subclasses, every one of them on untouched code.Deliberately out of scope: errors that appeared because an annotation became more accurate. The largest group follows r62529 giving
wpdb::get_col()and friends precise return types, which made pre-existing call-site looseness checkable for the first time. That code is as old as 2012 and unchanged; it is technical debt, not a regression. Same for the@return neverannotations that made long-standing defensive code visibly unreachable.Of the 63 distinct symbols and sites behind the remaining errors, 9 had changed since 7.0.0. Every genuine regression traced back to a type-annotation or code-quality commit rather than to feature work.
Committing to SVN
These commits may be landed in SVN separately rather than as one changeset. Each is self-contained: it changes one thing, regenerates the affected baseline, and leaves the tree green on its own. The table gives the revision(s) each would be a follow-up to.
string|voidonWP_Widget::form()voidremoved from a union tightened the contract; 18 subclassform()overrides echo and return nothing. 20 errors.WP_Block_Type::__get()'s typearray[]fromget_variations(), notvoidrestored.term_exists()'s conditional return for the no-taxonomy caseint|nullfor the empty-$taxonomybranch; that branch doesreturn (string) $_term.WP_Comment's two ID propertiesnumeric-stringis contradicted byget_comment_to_edit(), which casts both tointin place. Clears 39 baselined errors across 9 files.WP_Theme_JSON's private static methods throughselfstatic::, which resolves to the runtime class where a private method is not visible.get_feature_declarations_for_node()'s params as arraysobjectsince 6.3.0 but only ever passed arrays. The by-reference$nodepropagated the wrong type back to callers. 5 errors from one docblock.comment_shortcutsandinfinite_scrollingonWP_User__get()without being listed among the class's@propertytags, unlikerich_editingbeside it.$wpdbglobal on the Users screenisset()checks on properties that are always setisset()paired with! empty()/is_array()on declared properties with non-null defaults.merge_properties()array() !== $currentafter! array_is_list( $current ), which already implies non-empty.substr_compare()report PHPStan gets wrongfunctionMap.phpdrops the implicit nullability of$length; moved toignoreErrorswith the reasoning recorded.Previously committed
r63296 (abc50fe, "Formatting: Document that escaping functions can take numbers") came out of the same review and is already in trunk, so it is part of this branch's history rather than under review here. It widened the
esc_*()annotations tostring|int|floatand cleared 54 baseline entries. Worth noting it was not a regression fix: 87 of the 103 errors it resolved already existed at 7.0.0.Not fixed, and why
_upgrade_cron_array()(r62488) — the@phpstan-returnshape is correct about runtime behaviour, but building an array key-by-key and then settingversioncollapses PHPStan's inference tonon-empty-array<'version'|int, …>, losing the key/value correlation. Weakening a correct type to satisfy the analyzer seemed worse than leaving it baselined.WP_Postproperty reports (r62717) —$_wp_attachment_image_altis read through__get(), which accepts any meta key, so unlike theWP_Usercase above it cannot be resolved with a@propertytag.Verification
Every fix was checked with a full analysis before regenerating baselines, so that "no new errors" means the change introduced none rather than that regeneration absorbed them. Relevant suites were run per change: 316 theme.json, 794 comment, 104 block type, 78 view config, 46 block supports states, 26
term_exists.Trac ticket: Core-65817
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Building the 7.0.0-vs-trunk comparison, classifying each error by whether the symbol behind it changed after the tag, the fixes themselves, and drafting this description. Several of its intermediate classifications were wrong and were corrected after I pushed back on them; the scope decisions, and the final read of each fix, are mine.
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.