Remove return.missing baseline and fix its issue - #13082
Conversation
|
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. |
dmsnell
left a comment
There was a problem hiding this comment.
With a number of mixed categories of missing returns, this PR is probably too big to merge as a single change. Could we split it into those which should be benign by simply adding a missing return null; as the last statement in a function, from those which involve Docblock changes, from those which rearrange the return code (e.g. general-template.php, with the $display check).
those last ones seem trivial, but I fear from experience that it’s easy to accidentally flip a boolean and then introduce a regression
Resolve the conflict in the generated baseline include list in `phpstan.neon.dist`. Each side deleted a different baseline: this branch removed `return.missing.neon`, while trunk removed `return.empty.neon`. Both removals are kept, so neither include line survives and the list again matches the contents of `tests/phpstan/baselines/`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The annotation had been widened to `string|null|void`, but the `null` there is inert. PHPStan resolves `string|null|void` and `string|void` to the same call-site type, `string|null`, and suppresses `return.missing` on overrides in both cases; only `string|null` on its own makes the return obligatory. The union therefore behaved exactly as `string|void` while reading as though the stricter `null` spelling had been chosen. That distinction matters for this method. The base implementation echoes a notice and returns 'noform', while the 18 subclass overrides echo their own markup and fall off the end, four of them in bundled themes. `void` is the accurate description of that contract, and the one that keeps the overrides compliant. `string|null|void` was also the only union in `src/` to combine `null` and `void`, and it ordered `void` last where 45 of the 47 surviving `void` unions in core put it first. Matches the wording already used for this method on the `fix/phpstan-7.1-regressions` branch, and drops the blank line that had been inserted between `@param` and `@return`, which was the only such gap in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I'm preparing to make multiple commits for this. |
PHPStan reports `return.missing` where a documented return type obliges a function to return a value on every path, but one path falls off the end instead. Adding the `return null;` that the annotation already implies resolves the report without changing behavior, since PHP was returning null implicitly anyway. This is the shape r61716 used on `get_category_by_path()`, which replaced a bare `return;` in that same function; its remaining path is now handled too. Covered here are `get_category_by_path()`, `wp_list_users()`, `single_month_title()`, `WP_Customize_Manager::get_setting()`, `get_panel()`, `get_section()` and `get_control()`, `WP_Customize_Widgets::get_setting_type()`, `WP_Customize_Header_Image_Control::get_current_image_src()`, `WP_Image_Editor_Imagick::set_imagick_time_limit()`, and the four pagination helpers in `link-template.php`. Every change is a pure append, leaving the diff additive at 30 insertions and no deletions. Two further groups of `return.missing` fixes from the same pull request carry more risk and will follow separately. The baseline `tests/phpstan/baselines/return.missing.neon` drops from 41 entries covering 44 errors to 27 covering 29, so the file and its entry in `phpstan.neon.dist` both remain for now. Developed in #13082. Follow-up to r61716, r63020. Props dpantazis, dmsnell. See #65817. git-svn-id: https://develop.svn.wordpress.org/trunk@63378 602fd350-edb4-49c9-b593-d223f7449a82
Brings in r63369 through r63378, which includes the first of this branch's three groups of `return.missing` fixes: the 14 pure `return null;` appends landed on trunk as r63378, so they drop out of the branch's remaining diff. Resolve the modify/delete conflict on `tests/phpstan/baselines/return.missing.neon` in favour of the deletion. Trunk shrank the file to the 27 entries the appends did not cover; this branch still carries the fixes for all of them, so the file reaches zero and goes, taking its `phpstan.neon.dist` entry with it. Regenerating every baseline confirms it: `return.missing` no longer appears among the generated files, and no other baseline moved. What remains against trunk is the two groups held back for separate review, the `WP_Widget::form()` return type and the `$display` branch inversions in `general-template.php` and `link-template.php`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PHPStan reports `return.missing` where a documented return type obliges a function to return a value on every path, but one path falls off the end instead. Adding the `return null;` that the annotation already implies resolves the report without changing behavior, since PHP was returning null implicitly anyway. This is the shape r61716 used on `get_category_by_path()`, which replaced a bare `return;` in that same function; its remaining path is now handled too. Covered here are `get_category_by_path()`, `wp_list_users()`, `single_month_title()`, `WP_Customize_Manager::get_setting()`, `get_panel()`, `get_section()` and `get_control()`, `WP_Customize_Widgets::get_setting_type()`, `WP_Customize_Header_Image_Control::get_current_image_src()`, `WP_Image_Editor_Imagick::set_imagick_time_limit()`, and the four pagination helpers in `link-template.php`. Every change is a pure append, leaving the diff additive at 30 insertions and no deletions. Two further groups of `return.missing` fixes from the same pull request carry more risk and will follow separately. The baseline `tests/phpstan/baselines/return.missing.neon` drops from 41 entries covering 44 errors to 27 covering 29, so the file and its entry in `phpstan.neon.dist` both remain for now. Developed in WordPress/wordpress-develop#13082. Follow-up to r61716, r63020. Props dpantazis, dmsnell. See #65817. Built from https://develop.svn.wordpress.org/trunk@63378 git-svn-id: http://core.svn.wordpress.org/trunk@62571 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Nine template functions take a `$display` parameter and either echo their result or return it. Each is documented as returning `string|null`, but the echoing branch fell off the end without returning, which is what PHPStan reports as `return.missing`. Inverting the condition lets the value-returning path exit first and the echoing path fall through to an explicit `return null;`, the same shape `single_month_title()` received in r63378. Affected are `wp_title()`, `single_post_title()`, `post_type_archive_title()`, `single_term_title()`, `the_date()`, `the_modified_date()`, `edit_term_link()`, `next_posts()` and `previous_posts()`. No behavior changes. `WP_Widget::form()` is documented as `string|void` again. r59336 added the `void` deliberately, noting that unlike its siblings it is the child classes which return nothing when the method is correctly implemented. r62178 replaced it with `null` on the premise that `void` cannot belong to a union, which holds for PHP's native return types but not for PHPDoc, where PHPStan reads `void` in a union as "may not return at all". Under `string|null` the annotation instead obliged every override to return, putting all 18 of them in breach, two of which live in bundled themes that r62178 did not touch. With no occurrences left, `tests/phpstan/baselines/return.missing.neon` is deleted along with its entry in `phpstan.neon.dist`, completing the work begun in r63378. Developed in #13082. Follow-up to r59336, r62178, r63020, r63378. Props dpantazis, dmsnell. See #65817, #64704. git-svn-id: https://develop.svn.wordpress.org/trunk@63379 602fd350-edb4-49c9-b593-d223f7449a82
Nine template functions take a `$display` parameter and either echo their result or return it. Each is documented as returning `string|null`, but the echoing branch fell off the end without returning, which is what PHPStan reports as `return.missing`. Inverting the condition lets the value-returning path exit first and the echoing path fall through to an explicit `return null;`, the same shape `single_month_title()` received in r63378. Affected are `wp_title()`, `single_post_title()`, `post_type_archive_title()`, `single_term_title()`, `the_date()`, `the_modified_date()`, `edit_term_link()`, `next_posts()` and `previous_posts()`. No behavior changes. `WP_Widget::form()` is documented as `string|void` again. r59336 added the `void` deliberately, noting that unlike its siblings it is the child classes which return nothing when the method is correctly implemented. r62178 replaced it with `null` on the premise that `void` cannot belong to a union, which holds for PHP's native return types but not for PHPDoc, where PHPStan reads `void` in a union as "may not return at all". Under `string|null` the annotation instead obliged every override to return, putting all 18 of them in breach, two of which live in bundled themes that r62178 did not touch. With no occurrences left, `tests/phpstan/baselines/return.missing.neon` is deleted along with its entry in `phpstan.neon.dist`, completing the work begun in r63378. Developed in WordPress/wordpress-develop#13082. Follow-up to r59336, r62178, r63020, r63378. Props dpantazis, dmsnell. See #65817, #64704. Built from https://develop.svn.wordpress.org/trunk@63379 git-svn-id: http://core.svn.wordpress.org/trunk@62572 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
@dpanta94 I think the resolved Replacing I understand and support the effort to get rid of the cc @westonruter |
|
@IanDelMar Thank you for following up on this. I've opened #13359 to apply this feedback. |
✅ Committed in:
Removes in total
41errors.Removes errors from the below phpstan baselines and fixes the issues that they were covering:
How ?
Adds return types according to method docblocks.
Trac ticket: https://core.trac.wordpress.org/ticket/65817
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 5
Used for: Help with suggesting how to fix the phpstan output for the specific errors we are removing the baselines for. The actual result has been reviewed and are owned by me.
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.