Skip to content

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

Merged
ondrejmirtes merged 2 commits intophpstan:1.10.xfrom
staabm:serialize
May 4, 2023
Merged

Enforce magic __serialize() and __unserialize() return types#2372
ondrejmirtes merged 2 commits intophpstan:1.10.xfrom
staabm:serialize

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.

}

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

Development

Successfully merging this pull request may close these issues.

3 participants