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.

Sign up for GitHub

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

Jump to bottom

Increase phpstan from level 6 to level 8 #251

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Increase phpstan from level 6 to level 8 #251

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
25373c4
increase phpstan to level 7
phil-davis Jun 29, 2023
c8795a0
adjust test code to pass phpstan level 7
phil-davis Jun 29, 2023
ade9d87
Fix code for phpstan level 7
phil-davis Jun 29, 2023
0f54dcf
Fix code for phpstan level 8
phil-davis Jun 29, 2023
924cd8b
Check that callable exists in functionCaller
phil-davis Jun 29, 2023
afdfb31
Use is_callable to check the func parameter to functionCaller
phil-davis Jun 30, 2023
fe75c5e
Remove unneeded explode of func parameter to functionCaller
phil-davis Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion lib/Deserializer/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,8 @@
* this function.
*
* @return mixed whatever the 'func' callable returns
*
* @throws \InvalidArgumentException
*/
function functionCaller(Reader $reader, callable $func, string $namespace)
{
Expand All @@ -346,8 +348,22 @@
}

$funcArgs = [];
/**
* Even if $func gets "exploded" into an array, that array is still "callable"
* It should have elements that are the class and the function/method in the class.
*/
$func = is_string($func) && false !== strpos($func, '::') ? explode('::', $func) : $func;
$ref = is_array($func) ? new \ReflectionMethod($func[0], $func[1]) : new \ReflectionFunction($func);
if (is_callable($func)) {
if (is_array($func)) {
$ref = new \ReflectionMethod($func[0], $func[1]);
} elseif (($func instanceof \Closure) || is_string($func)) {
$ref = new \ReflectionFunction($func);
} else {
throw new \InvalidArgumentException(__METHOD__.' func parameter is not a callable array, string or closure.');
}
} else {
throw new \InvalidArgumentException(__METHOD__.' func parameter is not callable.');

Check warning on line 365 in lib/Deserializer/functions.php

View check run for this annotation

Codecov / codecov/patch

lib/Deserializer/functions.php#L365 

Added line #L365 was not covered by tests
Copy link
Contributor Author

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.

}
foreach ($ref->getParameters() as $parameter) {
$funcArgs[$parameter->getName()] = null;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Element/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

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.

*/
public static function xmlDeserialize(Xml\Reader $reader)
{
Expand Down
2 changes: 1 addition & 1 deletion lib/Element/Uri.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public function xmlSerialize(Xml\Writer $writer): void
{
$writer->text(
resolve(
$writer->contextUri,
$writer->contextUri ?? '',
Copy link
Contributor Author

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

$this->value
)
);
Expand Down
12 changes: 6 additions & 6 deletions lib/Reader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public function parse(): array
*
* @param array<string, mixed>|null $elementMap
*
* @return array<string, mixed>
* @return array<int,array<string, mixed>>
Copy link
Contributor Author

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.

*/
public function parseGetElements(array $elementMap = null): array
{
Expand All @@ -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
Copy link
Contributor Author

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.

*/
public function parseInnerTree(array $elementMap = null)
{
Expand Down Expand Up @@ -298,14 +298,14 @@ public function getDeserializerForElementName(string $name): callable
}

$deserializer = $this->elementMap[$name];
if (is_subclass_of($deserializer, 'Sabre\\Xml\\XmlDeserializable')) {
return [$deserializer, 'xmlDeserialize'];
}

if (is_callable($deserializer)) {
return $deserializer;
}

if (is_subclass_of($deserializer, 'Sabre\\Xml\\XmlDeserializable')) {
Copy link
Contributor Author

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.

return [$deserializer, 'xmlDeserialize'];
}

$type = gettype($deserializer);
if (is_string($deserializer)) {
$type .= ' ('.$deserializer.')';
Expand Down
1 change: 1 addition & 0 deletions lib/Service.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ?? '';
Copy link
Contributor Author

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.


$this->elementMap[$elementName] = function (Reader $reader) use ($className, $namespace) {
return \Sabre\Xml\Deserializer\valueObject($reader, $className, $namespace);
Expand Down
4 changes: 3 additions & 1 deletion lib/Writer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ public function startElement($name): bool
if ('{' === $name[0]) {
list($namespace, $localName) =
Service::parseClarkNotation($name);
$namespace = $namespace ?? '';

if (array_key_exists($namespace, $this->namespaceMap)) {
$result = $this->startElementNS(
Expand All @@ -134,7 +135,7 @@ public function startElement($name): bool
} else {
// An empty namespace means it's the global namespace. This is
// allowed, but it mustn't get a prefix.
if ('' === $namespace || null === $namespace) {
if ('' === $namespace) {
$result = $this->startElement($localName);
$this->writeAttribute('xmlns', '');
} else {
Expand Down Expand Up @@ -238,6 +239,7 @@ public function writeAttribute($name, $value): bool
$localName
) = Service::parseClarkNotation($name);

$namespace = $namespace ?? '';
if (array_key_exists($namespace, $this->namespaceMap)) {
// It's an attribute with a namespace we know
return $this->writeAttribute(
Expand Down
2 changes: 1 addition & 1 deletion phpstan.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ includes:
- phpstan-baseline.neon

parameters:
level: 6
level: 8
phpVersion: 70430 # PHP 7.4.30
paths:
- lib
Expand Down
15 changes: 11 additions & 4 deletions tests/Sabre/Xml/ContextStackTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,30 @@ public function setUp(): void
public function testPushAndPull(): void
{
$this->stack->contextUri = '/foo/bar';
$this->stack->elementMap['{DAV:}foo'] = 'Bar';
// Use a class that exists so that phpstan will be happy that the value of the elementMap
// element is a class-string. That is a type that is expected for elementMap in ContextStackTrait.
$testClass = 'Sabre\Xml\ContextStack';
if (class_exists($testClass)) {
$this->stack->elementMap['{DAV:}foo'] = $testClass;
} else {
self:self::fail("$testClass not found");
}
$this->stack->namespaceMap['DAV:'] = 'd';

$this->stack->pushContext();

self::assertEquals('/foo/bar', $this->stack->contextUri);
self::assertEquals('Bar', $this->stack->elementMap['{DAV:}foo']);
self::assertEquals($testClass, $this->stack->elementMap['{DAV:}foo']);
self::assertEquals('d', $this->stack->namespaceMap['DAV:']);

$this->stack->contextUri = '/gir/zim';
$this->stack->elementMap['{DAV:}foo'] = 'newBar';
$this->stack->elementMap['{DAV:}foo'] = 'stdClass';
$this->stack->namespaceMap['DAV:'] = 'dd';

$this->stack->popContext();

self::assertEquals('/foo/bar', $this->stack->contextUri);
self::assertEquals('Bar', $this->stack->elementMap['{DAV:}foo']);
self::assertEquals($testClass, $this->stack->elementMap['{DAV:}foo']);
self::assertEquals('d', $this->stack->namespaceMap['DAV:']);
}
}
6 changes: 6 additions & 0 deletions tests/Sabre/Xml/ServiceTest.php
View file
Open in desktop
phil-davis marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use PHPUnit\Framework\TestCase;
use Sabre\Xml\Element\KeyValue;

use function PHPUnit\Framework\assertIsResource;

class ServiceTest extends TestCase
{
public function testGetReader(): void
Expand Down Expand Up @@ -90,6 +92,7 @@ public function testParseStream(): void
</root>
XML;
$stream = fopen('php://memory', 'r+');
self:assertIsResource($stream);
fwrite($stream, $xml);
rewind($stream);

Expand Down Expand Up @@ -201,6 +204,7 @@ public function testExpectStream(): void
XML;

$stream = fopen('php://memory', 'r+');
self:assertIsResource($stream);
fwrite($stream, $xml);
rewind($stream);

Expand Down Expand Up @@ -285,6 +289,7 @@ public function testMapValueObject(): void
$orderService->namespaceMap[$ns] = null;

$order = $orderService->parse($input);
self::assertIsObject($order);
$expected = new Order();
$expected->id = 1234;
$expected->amount = 99.99;
Expand Down Expand Up @@ -324,6 +329,7 @@ public function testMapValueObjectArrayProperty(): void
$orderService->namespaceMap[$ns] = null;

$order = $orderService->parse($input);
self::assertIsObject($order);
$expected = new Order();
$expected->id = 1234;
$expected->amount = 99.99;
Expand Down