Conversation
_get_list_table() instantiates whichever class `$class_name` names, but the docblock only promised `WP_List_Table|false`. Every caller reaching for a member the subclass adds was therefore an error to PHPStan and got suppressed in a baseline: `WP_Posts_List_Table::inline_edit()` and `WP_Terms_List_Table::inline_edit()`, `WP_Privacy_Requests_Table::process_bulk_action()` and `::embed_scripts()`, `WP_Theme_Install_List_Table::theme_installer_single()`, and the extra parameters that `WP_Terms_List_Table::single_row()`, `WP_Users_List_Table::single_row()`, `WP_Posts_List_Table::display_rows()` and `WP_Post_Comments_List_Table::display()` accept over their parent signatures. A `@phpstan-template` bound to `WP_List_Table` lets the return type follow the `class-string` that was passed in, which is what the function has always done at runtime. Eleven baseline entries covering twelve errors resolve as a result: all seven `method.nonObject` entries, emptying that baseline so the file is deleted along with its `includes` entry in phpstan.neon.dist, and four `arguments.count` entries. The three `property.protected` entries for `$screen` are the same three accesses as before and are re-baselined only because the message now names the concrete subclass rather than `WP_List_Table`. The `@return` tag keeps its `WP_List_Table|false` type so the generated developer documentation is unchanged; only its description gains a note about the type being the one requested.
|
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. |
| * | ||
| * @phpstan-template T of WP_List_Table | ||
| * @phpstan-param class-string<T> $class_name |
There was a problem hiding this comment.
This fixes the following PHPStan error:
Function _get_list_table() should return (T of WP_List_Table)|false but returns object.
| * @param array $args { | ||
| * Optional. Arguments to pass to the class. | ||
| * | ||
| * @type string $plural Plural value used for labels and the objects being listed. | ||
| * This affects things such as CSS class-names and nonces used | ||
| * in the list table, e.g. 'posts'. Default empty. | ||
| * @type string $singular Singular label for an object being listed, e.g. 'post'. | ||
| * Default empty | ||
| * @type bool $ajax Whether the list table supports Ajax. This includes loading | ||
| * and sorting data, for example. If true, the class will call | ||
| * the _js_vars() method in the footer to provide variables | ||
| * to any scripts handling Ajax events. Default false. | ||
| * @type string $screen String containing the hook name used to determine the current | ||
| * screen. If left null, the current screen will be automatically set. | ||
| * Default null. | ||
| * } |
There was a problem hiding this comment.
Adding this fixes the following argument.type PHPStan error:
Parameter #1 $args of class WP_List_Table constructor expects array{plural?: string, singular?: string, ajax?: bool, screen?: string, ...}|string, non-empty-array given.
There was a problem hiding this comment.
Also a missingType.iterableValue error:
Function _get_list_table() has parameter $args with no value type specified in iterable type array.
There was a problem hiding this comment.
And on:
wordpress-develop/src/wp-admin/includes/list-table.php
Lines 72 to 73 in 0587894
It fixes an argument.type error:
Parameter #1 $hook_name of function convert_to_screen expects string, mixed given.
There was a problem hiding this comment.
Typing out the $args exposed this bug where WP_List_Table is supposed to take a screen arg which is string|null, but the _get_list_table() and other callers are passing WP_Screen. This seems out of scope here, but it's why the new entries are added to the baseline.
There was a problem hiding this comment.
Actually, it wasn't hard to fix this. Addressed in 2fb1b42
Adding the hash notation to $args dropped the padding that lined its type and name up with $class_name. Core aligns those columns even when the hash notation param is last. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WP_List_Table::__construct() defaults its screen argument to null and hands that value straight to convert_to_screen(), which forwards it to WP_Screen::get(). Both fall through to the $hook_suffix global when given null, so null has always been accepted; only the two docblocks said otherwise, contradicting the hash notation that now documents the argument as string|WP_Screen|null. Also rewords the note about instances being returned unchanged, which read awkwardly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Default empty" on $singular has been missing its period since the hash notation was written; copying the block into _get_list_table() duplicated it, so fix both. "or the current screen instance" reads as though only the screen currently in scope qualifies. Any WP_Screen does, which is how convert_to_screen() now words it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- The return type promised `WP_List_Table|false` whatever class was asked for. A `@phpstan-template` now ties it to the `class-string` passed in. - The `$args` param was described as accepting only `screen`. It now carries the hash notation from `WP_List_Table::__construct()`. - That `screen` argument was typed as a string. It is documented as the `string|WP_Screen|null` it has always accepted. This type union is also applied to `convert_to_screen()` and `WP_Screen::get()`. Nothing changes at runtime. The function is left free of PHPStan errors at every rule level. Developed in #13553. Follow-up to r29459, r54378, r63020, r63420. Props soean, westonruter. See #65817. git-svn-id: https://develop.svn.wordpress.org/trunk@63648 602fd350-edb4-49c9-b593-d223f7449a82
- The return type promised `WP_List_Table|false` whatever class was asked for. A `@phpstan-template` now ties it to the `class-string` passed in. - The `$args` param was described as accepting only `screen`. It now carries the hash notation from `WP_List_Table::__construct()`. - That `screen` argument was typed as a string. It is documented as the `string|WP_Screen|null` it has always accepted. This type union is also applied to `convert_to_screen()` and `WP_Screen::get()`. Nothing changes at runtime. The function is left free of PHPStan errors at every rule level. Developed in WordPress/wordpress-develop#13553. Follow-up to r29459, r54378, r63020, r63420. Props soean, westonruter. See #65817. Built from https://develop.svn.wordpress.org/trunk@63648 git-svn-id: http://core.svn.wordpress.org/trunk@62823 1a063a9b-81f0-0310-95a4-ce76da25c4cd
2 participants
✅ Committed in r63648 (cf0ad1e).
_get_list_table()returnsnew $class_name( $args ), but the docblock only promised the base class. PHPStan therefore sawWP_List_Table|falseat every call site and reported each use of a subclass member as an error, all of which were suppressed in baselines. A@phpstan-templatebound toWP_List_Tableties the return type to theclass-stringthat was passed in. The@returntag keepsWP_List_Table|false, so the generated developer documentation is unaffected.The
wp_list_table_class_namefilter from [54378] (#18449) can substitute a different class; since it exists so extenders can swap in a replacement list table, the annotation treats the substitute as satisfyingT.Effect on the baselines
Eleven entries covering twelve errors resolve: all 7
method.nonObjectand 4arguments.count. That emptiesmethod.nonObject, so as its header directs the file is deleted along with itsincludesentry in phpstan.neon.dist.The 3
property.protectedentries change message but not substance — same accesses, same lines, totals unchanged at 8 entries and 11 errors. Only the class named in the message moves fromWP_List_Tableto the concrete subclass. Fixing those accesses is separate work.Addendum: the remaining level 10 errors
Later commits take the function to zero PHPStan level 10 errors. Everything still reported came from
$args, documented only asOptional. Arguments to pass to the class. Accepts 'screen'.$argsnow carries the hash notation fromWP_List_Table::__construct(). "Accepts 'screen'" understated it:wp-admin/export-personal-data.phpandwp-admin/erase-personal-data.phpboth passpluralandsingular, and those reach the base constructor because the privacy request tables do not override it.HashNotationVisitorturns the@typelist into an array shape, so the argument is typed at every call site instead of being a barearray. The shape is unsealed, so callers passing keys that only a subclass consumes are unaffected.screenaccepts aWP_Screen, not only a hook name. Typing the shape immediately produced fourargument.typeerrors —wp-admin/includes/ajax-actions.phptwice,wp-admin/includes/update.php, and the constructor call inside_get_list_table()itself — all of which pass or forwardget_current_screen(). That has always worked:_get_list_table()runs the value throughconvert_to_screen(), which forwards toWP_Screen::get(), which returns aWP_Screenit is handed unchanged and already documentedstring|WP_Screen. Only the docblocks in between were narrower than the code.WP_List_Table::__construct(),_get_list_table(),convert_to_screen()andWP_Screen::get()now agree onstring|WP_Screen|null. Thenullis there because the constructor defaults the argument tonulland hands that value straight down, and both functions fall through to the$hook_suffixglobal when they receive it. No behavior changes.The
wp_list_table_class_namedocblock carries the template too, as@phpstan-template T of WP_List_Tableand@phpstan-param class-string<T> $class_name. Without them the filtered value is a plainstringandreturn new $class_name( $args )reportsFunction _get_list_table() should return (T of WP_List_Table)|false but returns object.Worth noting that this is a level 10 report on a line the branch does not touch, so neither the level 5 CI run nor a diff-scoped check surfaces it.The baselines are unchanged from what the first commits left them at: the two entries these changes briefly needed were removed again once the docblocks were widened.
Noted for a follow-up ticket
Four call sites carry an inline
@varhint on the result — two inwp-admin/includes/update.php, two inwp-admin/includes/ajax-actions.php. Before this PR they did two jobs: naming the subclass and dropping the|false. The template now does the first, leaving them as pure|falsesuppression.Each of those four passes a literal class name that is in
$core_classes, wherefalseis unreachable —_get_list_table()returnsfalseonly for a name that is not in the map, and then it returnsfalseunconditionally rather than an instance. So the honest contract is a conditional@phpstan-returnkeyed on the known names, which resolves those calls to the bare instance type and lets all four hints be deleted; that checks out against the full suite. It also duplicates the seventeen class names into the docblock with nothing keeping them in sync, and a name dropped from the array but left in the docblock would fail silently in the unsafe direction. That trade deserves its own ticket rather than riding along here.Trac ticket: https://core.trac.wordpress.org/ticket/65817
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.