-
Notifications
You must be signed in to change notification settings - Fork 242
Bump to PHP 7.4+ and upgrade to PHPStan v2 #639
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
Conversation
message: '#^Parameter \#1 \$value of function strval expects bool\|float\|int\|resource\|string\|null, mixed given\.$#' | ||
identifier: argument.type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple lines here are changed just because PHPStan v2 does no longer double-escape, and adds the identifier of the error. I'll comment only on new ignored issues.
phpstan-baseline.neon
Outdated
- | ||
message: '#^Method Prophecy\\Doubler\\CachedDoubler\:\:createDoubleClass\(\) should return class\-string\<Prophecy\\Doubler\\DoubleInterface&T of object\> but returns class\-string\.$#' | ||
identifier: return.type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a method extending its parent which is generic. It seems that the nested-double generic isn't working, we could report this upstream to PHPStan
phpstan-baseline.neon
Outdated
message: '#^Call to function method_exists\(\) with ''ReflectionClass'' and ''isReadOnly'' will always evaluate to true\.$#' | ||
identifier: function.alreadyNarrowedType | ||
count: 1 | ||
path: src/Prophecy/Doubler/Generator/ClassMirror.php | ||
|
||
- | ||
message: '#^Call to function method_exists\(\) with ReflectionMethod and ''hasTentativeReturnT…'' will always evaluate to true\.$#' | ||
identifier: function.alreadyNarrowedType | ||
count: 1 | ||
path: src/Prophecy/Doubler/Generator/ClassMirror.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are due to the fact that those reflection methods are present only from PHP 8.1. These workaround go away as soon as we bump the minimum PHP version.
phpstan-baseline.neon
Outdated
message: '#^PHPDoc tag @var with type static\(Prophecy\\Doubler\\LazyDouble\<U of object\>\) is not subtype of native type \$this\(Prophecy\\Doubler\\LazyDouble\<T of object\>\)\.$#' | ||
identifier: varTag.nativeType | ||
count: 1 | ||
path: src/Prophecy/Doubler/LazyDouble.php | ||
|
||
- | ||
message: '#^Property Prophecy\\Doubler\\LazyDouble\:\:\$class \(ReflectionClass\<T of object\>\|null\) does not accept ReflectionClass\<U of object\>\.$#' | ||
identifier: assign.propertyType | ||
count: 1 | ||
path: src/Prophecy/Doubler/LazyDouble.php | ||
|
||
- | ||
message: '#^Property Prophecy\\Doubler\\LazyDouble\:\:\$class \(ReflectionClass\<T of object\>\|null\) is never assigned ReflectionClass\<T of object\> so it can be removed from the property type\.$#' | ||
identifier: property.unusedType | ||
count: 1 | ||
path: src/Prophecy/Doubler/LazyDouble.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 are all from the same issue, the setParentClass method is very hard to map against PHPStan because it changes the type of the generic of the instance itself.
- | ||
message: '#^Call to function method_exists\(\) with ReflectionMethod and ''hasTentativeReturnT…'' will always evaluate to true\.$#' | ||
identifier: function.alreadyNarrowedType | ||
count: 1 | ||
path: src/Prophecy/Prophecy/MethodProphecy.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, 8.1+ reflection feature...
@@ -22,7 +22,7 @@ class UnexpectedCallsCountException extends UnexpectedCallsException | |||
* @param string $message | |||
* @param MethodProphecy $methodProphecy | |||
* @param int $count | |||
* @param list<Call> $calls | |||
* @param array<Call> $calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inputing any array, casting to list below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this, or should we use a more precise type in our CallCenter (which is probably already using lists) ?
* @param array<Call> $calls | ||
*/ | ||
public function __construct($message, MethodProphecy $methodProphecy, array $calls) | ||
{ | ||
parent::__construct($message, $methodProphecy); | ||
|
||
$this->calls = $calls; | ||
$this->calls = array_values($calls); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, casting to list (previous was extending class)
@@ -48,7 +48,7 @@ public function check(array $calls, ObjectProphecy $object, MethodProphecy $meth | |||
{ | |||
$callback = $this->callback; | |||
|
|||
if ($callback instanceof Closure && method_exists('Closure', 'bind') && (new ReflectionFunction($callback))->getClosureThis() !== null) { | |||
if ($callback instanceof Closure && (new ReflectionFunction($callback))->getClosureThis() !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bind
is present since PHP 5.4, we can ditch this check.
@@ -49,7 +49,7 @@ public function execute(array $args, ObjectProphecy $object, MethodProphecy $met | |||
{ | |||
$callback = $this->callback; | |||
|
|||
if ($callback instanceof Closure && method_exists('Closure', 'bind') && (new ReflectionFunction($callback))->getClosureThis() !== null) { | |||
if ($callback instanceof Closure && (new ReflectionFunction($callback))->getClosureThis() !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bind
is present since PHP 5.4, we can ditch this check.
@@ -193,6 +190,7 @@ protected static function recursiveExport(&$value, $indentation, $processed = nu | |||
|
|||
$values = "\n".$values.$whitespace; | |||
} | |||
\assert(\is_object($value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a change in how PHPStan detect types; better but not completely.
@MarcelloDuarte I've bumped to PHP 7.4 as discussed. I've also fixed the PHPStan analysis, but I saw that the output depends on the PHP version it runs on. It seems acceptable for now. I also added a changelog entry, so that a 1.22 should be released with this bump. Let me know if this is ok or if it needs more work. |
@@ -281,6 +281,6 @@ function (string $type) use ($class) { | |||
$types[] = 'null'; | |||
} | |||
|
|||
return $types; | |||
return array_values($types); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually an issue in phpstan IMO. It should know that ReflectionUnionType::getTypes
returns a list (so that doing array_map
on it still returns a list)
src/Prophecy/Util/ExportUtil.php
Outdated
@@ -147,7 +147,6 @@ protected static function recursiveExport(&$value, $indentation, $processed = nu | |||
return 'Array &'.$key; | |||
} | |||
|
|||
\assert(\is_array($value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this removal causing the issues reported just below ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this PR unblocking phpspec ? Removing support for PHP versions in a dependency cannot be a blocker (phpspec requires that prophecy supports at least all the PHP versions it needs, but does not care if it supports more).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's some sort of circular dependency around stan. That wasn't my question though... Is this mergeable? Do we need to keep supporting PHP versions in EOL state to keep the next tag within minor release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep supporting PHP versions in EOL state to keep the next tag within minor release?
As long as we update our php
requirement properly in the composer.json, dropping support for some PHP versions is not a BC break (as composer will still respect that requirement when resolving semver ranges). That's still something that deserves a minor version IMO (it is not a bugfix), but it does not require a major version bump as far as Composer is concerned (Symfony itself has a policy to be stricter for the fullstack framework, but that's for other reasons)
@MarcelloDuarte this is the PR that I promised. I'll self-review to add more context to each change.