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

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

Merged
merged 2 commits into from May 4, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 4, 2023

types as defined in php-src

https://www.php.net/manual/en/language.oop5.magic.php#language.oop5.magic.serialize
https://3v4l.org/bLjLG

my motivation for this change is, as I am upgrading a php7 app for php8 compat per https://wiki.php.net/rfc/phase_out_serializable and I experienced php segfaults when I use the wrong signature or return the wrong data

returning the wrong result is a fatal error on php8

@staabm staabm marked this pull request as ready for review May 4, 2023 10:28
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@@ -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

@ondrejmirtes ondrejmirtes merged commit 6682a4a into phpstan:1.10.x May 4, 2023
374 of 377 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the serialize branch May 4, 2023 13:38
@staabm staabm mentioned this pull request May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants