Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with _get_list_table() #261

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

IanDelMar
Copy link
Contributor

This PR fixes the return types of _get_list_table() and resolves the "not always the same as T. It breaks the contract for some argument types, typically subtypes" issue (see at phpstan.org/try). The PHPStan version is updated to 1.11 to support new<T>.

Compared to the current implementation, we lose the check for whether $class_name is a class-string. Once we drop support for PHPStan 1.x, we can update '@phpstan-template T' => 'of string' to '@phpstan-template T' => 'of class-string' to restore the check that $class_name is a class-string. I don't think that new<T> is supported by Psalm.

Once merged, #256 can be closed.

Fixes #257
Related to #256

Another option to resolve this is to revert migrating the dynamic return type extension from szepeviktor/phpstan-wordpress.

cc: @johnbillion

@szepeviktor
Copy link
Member

This is a great PR.

Waiting for John ... and @swissspidy

@IanDelMar
Copy link
Contributor Author

Perhaps @kkmuffme would also like to have a look regarding the Psalm support. Any feedback would be greatly appreciated.

@johnbillion
Copy link
Contributor

I don't have time to look deeply into this at the moment. I trust Ian's judgement.

@szepeviktor szepeviktor merged commit ed07e4c into php-stubs:master Nov 12, 2024
6 checks passed
@szepeviktor
Copy link
Member

We have just taken off without landing gears.

@IanDelMar IanDelMar deleted the fix-get-list-table branch November 12, 2024 14:17
@kkmuffme
Copy link

So basically this PR is only needed bc phpstan 1 does not support class-string in template and would be reverted later? Not sure if that change is a good idea to begin with then.

@szepeviktor
Copy link
Member

Anyone able to answer?

@IanDelMar
Copy link
Contributor Author

IanDelMar commented Nov 25, 2024

So basically this PR is only needed bc phpstan 1 does not support class-string in template and would be reverted later? Not sure if that change is a good idea to begin with then.

This PR was necessary because we were encountering incorrect return types. Unfortunately, I was unable to resolve the issues with _get_list_table() by any other means. Admittedly, I am not an expert, and the only alternative I could think of was to remove it entirely and revert to providing the return types via the return type extension in szepeviktor/phpstan-wordpress. I am, however, open to alternative suggestions and happy to consider any advice you might have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_get_list_table issues
4 participants