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

[11.x] Update PHPStan to 2.x #53716

Draft
wants to merge 6 commits into
base: 11.x
Choose a base branch
from
Draft

Conversation

tamiroh
Copy link
Contributor

@tamiroh tamiroh commented Nov 30, 2024

PHPStan 2.0 has been released so this pull request updates PHPStan version to 2.x.

Most of the fixes are under types/.

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@tamiroh tamiroh changed the title [11.x] Use PHPStan 2.0 [11.x] Update PHPStan to 2.x Nov 30, 2024
@tamiroh tamiroh marked this pull request as ready for review November 30, 2024 13:15
@taylorotwell
Copy link
Member

All those hard-coded integers seem kinda weird?

@taylorotwell taylorotwell marked this pull request as draft November 30, 2024 22:33
@crynobone crynobone requested a review from nunomaduro December 10, 2024 08:18
@@ -258,6 +258,7 @@ public function run(array|string|null $command = null, ?callable $output = null)

return new ProcessResult(tap($process)->run($output));
} catch (SymfonyTimeoutException $e) {
/** @phpstan-ignore variable.undefined */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring errors doesn't seem right.

Maybe move $process = $this->toSymfonyProcess($command); out of the try section instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Fixed in 7ce889a.

@@ -4,7 +4,7 @@

use function PHPStan\Testing\assertType;

new class implements ValidationRule
$class = new class implements ValidationRule
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$class = new class implements ValidationRule
new class implements ValidationRule

Copy link
Contributor Author

@tamiroh tamiroh Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instance must be assigned to some variable to avoid the following error...

 ------ ----------------------------------------------------------------------------------------------------------------------------- 
  Line   ValidationRule.php                                                                                                           
 ------ ----------------------------------------------------------------------------------------------------------------------------- 
  7      Expression "new class implements \Illuminate\Contracts\Validation\ValidationRule…" on a separate line does not do anything.  
         🪪  expr.resultUnused                                                                                                        
 ------ ----------------------------------------------------------------------------------------------------------------------------- 

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is valid, but the real problem is something else

Looking at the entire file, it doesn't seem like it's testing anything. The class is being instantiated, but the assertion call is inside a method that isn't being called.

I'd rather see the test being reworked so there's a point in even having it.

Comment on lines +24 to 26
assertType('36', $cache->remember('cache', now(), function (): int {
return 36;
}));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume that a callback with int return type would result in a type called int and not 36. There seems to be a problem elsewhere.

All these type assertions should remain unchanged.

Copy link
Contributor Author

@tamiroh tamiroh Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that since PHPStan 2.0, types are inferred more narrowly, in this case as 36 literal type.

https://phpstan.org/writing-php-code/phpdoc-types#literals-and-constants

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that's something that can be configured somewhere, since the returned type is a string with the number, so the "literal" part is misleading.

@tamiroh tamiroh requested a review from sydgren December 22, 2024 10:41
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.

3 participants