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

Enforce magic __serialize() and __unserialize() return types #2372

Merged
merged 2 commits into from
May 4, 2023
Merged

Enforce magic __serialize() and __unserialize() return types #2372

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba0c28f
Enforce magic __serialize() and __unserialize() return types
staabm May 4, 2023
32199cd
fix test
staabm May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Reflection/Php/PhpMethodFromParserNodeReflection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use PHPStan\Type\BooleanType;
use PHPStan\Type\Generic\TemplateTypeMap;
use PHPStan\Type\IntegerType;
use PHPStan\Type\MixedType;
use PHPStan\Type\ObjectWithoutClassType;
use PHPStan\Type\StringType;
use PHPStan\Type\Type;
Expand Down Expand Up @@ -78,6 +79,13 @@ public function __construct(
$realReturnType = TypeCombinator::intersect(new ObjectWithoutClassType(), $realReturnType);
}

if ($name === '__unserialize') {
$realReturnType = new VoidType();
Copy link
Member

Choose a reason for hiding this comment

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

This change would make sense if there's a type missing in the signature, like: https://3v4l.org/Wji2j

So please change the current test and remove the types from it.

But we need another rule and test - that checks the correctness of the signatures. In the example with __unserialize(string $data): string, both parameter and return type are wrong, but these aren't picked up by any rule.

While we're at it we should verify the corectness of signatures of all magic methods. Needs a new rule probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you.

So please change the current test and remove the types from it.

I have fixed the test as per your feedback

But we need another rule and test - that checks the correctness of the signatures. In the example with __unserialize(string $data): string, both parameter and return type are wrong, but these aren't picked up by any rule.
While we're at it we should verify the corectness of signatures of all magic methods. Needs a new rule probably.

I will prepare a separate PR for that

}
if ($name === '__serialize') {
$realReturnType = new ArrayType(new MixedType(), new MixedType());
}

parent::__construct(
$classMethod,
$fileName,
Expand Down
14 changes: 14 additions & 0 deletions tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,4 +861,18 @@ public function testBug9011(): void
$this->analyse([__DIR__ . '/data/bug-9011.php'], $errors);
}

public function testMagicSerialization(): void
{
$this->analyse([__DIR__ . '/data/magic-serialization.php'], [
[
'Method MagicSerialization\WrongSignature::__serialize() should return array but returns string.',
23,
],
[
'Method MagicSerialization\WrongSignature::__unserialize() with return type void returns string but should not return anything.',
28,
],
]);
}

}
30 changes: 30 additions & 0 deletions tests/PHPStan/Rules/Methods/data/magic-serialization.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

namespace MagicSerialization;

class Ok {

/** @return array<mixed> */
public function __serialize(): array
{
return [];
}

/** @param array<mixed> $data */
public function __unserialize(array $data): void
{
}
}

class WrongSignature {

public function __serialize(): string
{
return '';
}

public function __unserialize(string $data): string
{
return '';
}
}