-
Notifications
You must be signed in to change notification settings - Fork 77
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
Increase phpstan from level 6 to level 8 #251
Increase phpstan from level 6 to level 8 #251
Conversation
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
============================================
- Coverage 96.90% 96.75% -0.16%
+ Complexity 116 115 -1
============================================
Files 13 13
Lines 485 493 +8
============================================
+ Hits 470 477 +7
- Misses 15 16 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -73,7 +73,7 @@ public function xmlSerialize(Xml\Writer $writer): void | |||
* $reader->parseInnerTree() will parse the entire sub-tree, and advance to | |||
* the next element. | |||
* | |||
* @return array<string, mixed>|string|null | |||
* @return array<int,array<string, mixed>>|string|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.
This was an existing error in the PHPdoc. This thing returns an array of arrays.
@@ -105,7 +105,7 @@ public function parse(): array | |||
* | |||
* @param array<string, mixed>|null $elementMap | |||
* | |||
* @return array<string, mixed> | |||
* @return array<int,array<string, mixed>> |
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 was an existing error in the PHPdoc. This thing returns an array of arrays.
@@ -130,7 +130,7 @@ public function parseGetElements(array $elementMap = null): array | |||
* | |||
* @param array<string, mixed>|null $elementMap | |||
* | |||
* @return array<string, mixed>|string|null | |||
* @return array<int,array<string, mixed>>|string|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.
This was an existing error in the PHPdoc. This thing returns an array of arrays.
@@ -59,7 +59,7 @@ public function xmlSerialize(Xml\Writer $writer): void | |||
{ | |||
$writer->text( | |||
resolve( | |||
$writer->contextUri, | |||
$writer->contextUri ?? '', |
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.
contextUri
starts life set to null
.
phpstan can't be sure that it is no longer null
The safest thing here is to pass the empty string if it is currently null
if (is_callable($deserializer)) { | ||
return $deserializer; | ||
} | ||
|
||
if (is_subclass_of($deserializer, 'Sabre\\Xml\\XmlDeserializable')) { |
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.
is_subclass_of
takes a class name or object in parameter 1.
But $deserializer
can be a callable function.
phpstan is happy if the check for is_callable
comes first.
@@ -241,6 +241,7 @@ public function write(string $rootElementName, $value, string $contextUri = null | |||
public function mapValueObject(string $elementName, string $className): void | |||
{ | |||
list($namespace) = self::parseClarkNotation($elementName); | |||
$namespace = $namespace ?? ''; |
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.
parseClarkNotation
says that it can return string|null
in the first element of the returned array.
later uses of $namespace
expect a string
. So just convert null
to the empty string here.
Same for a few place in Writer.php below.
I will also try to add some unit tests that pass non-existent functions/methods to |
lib/Deserializer/functions.php
Outdated
throw new \InvalidArgumentException(__METHOD__.' func parameter is not a callable array, string or closure.'); | ||
} | ||
} else { | ||
throw new \InvalidArgumentException(__METHOD__.' func parameter is not callable.'); |
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.
I can't see a way to get to this code.
When I try to make a unit test that passes a function reference like "SomeValidClass::unknownMethod", then PHP notices that during the call to functionCaller
. Even though the string passed looks like a reasonable format for a callable thing, at run-time PHP checks if the method exists in the class. And the test fails with:
TypeError: Argument 2 passed to Sabre\Xml\Deserializer\functionCaller() must be callable, string given, called in /home/phil/git/sabre-io/xml/tests/Sabre/Xml/Deserializer/FunctionCallerTest.php on line 154
So PHP really verifies that the thing passed in as parameter $func
not only looks like it could be callable, but that it is actually callable.
So in real-life we don't need all this double-checking.
I will see if there is a simpler way of coding this and keeping phpstan happy at the same time.
// ReflectionFunction can take either of those | ||
$ref = new \ReflectionFunction($func); | ||
} else { | ||
throw new \InvalidArgumentException(__METHOD__.' unable to use func parameter with ReflectionMethod or ReflectionFunction.'); |
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.
I can't cover this line with a unit test.
My analysis is:
PHP always checks that $func
is an actual callable that will work (it doesn't just check that $func
has a format that looks like it should be callable, does actual check that something like "MyClass::someMethod" really is a function that exists in a class that exists.
So when we get to this "if" block, we know that $func
is a working callable.
If it is in array form, then we call ReflectionMethod
If it is in string form with a "::" in it, then we also call ReflectionMethod
If it is a real Closure, or a string without "::" in it then we call ReflectionFunction
(because it is an ordinary function)
And those are all the ways that a "callable" can be passed.
But if I do not have an "else" to catch Unicorns, then phpstan
complains that $ref
might not be defined.
I think that it is a computable task for phpstan
to analyze that $ref
must be defined. phpstan
has to understand what are all the things whose union is a "callable". Then it can deduce that the if-elseif-... covers them all.
@staabm I reorganized the if-else... block in I thin kthis is enough on this! Please review and I will merge if OK. |
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.
thank you
This seems to be possible without breaking the code ;)