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

Changed signature of Serializable::unserialize stub #6438

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

SCIF
Copy link
Contributor

@SCIF SCIF commented Sep 6, 2021

@SCIF
Copy link
Contributor Author

SCIF commented Sep 6, 2021

I can see there are some test crashes but want to know if @orklah and @weirdan are fine with the PR prior fixing tests.

@orklah
Copy link
Collaborator

orklah commented Sep 6, 2021

There is a concrete difference between phpdoc and types in signature, even in stubs.

To have a compatible implementation, you have to respect types in parent signature according to two rules:

  • You can always declare a parameter type with a wider type
  • You can always declare a return type with a more narrow type

This means that when you add a param type in signature, the implementation may keep its type empty (because empty is mixed and mixed is wider). However, as soon as you declare a return type, you require that each implementation declare a return type at least equal to what you added. This is why tests fail.

In conclusion, you may keep types in signature for parameters but you have to keep them in phpdoc for return types

@SCIF
Copy link
Contributor Author

SCIF commented Sep 6, 2021

@orklah , @weirdan ,
Guys, don't you think, this one is the outdated test and should be deleted?

                '<?php
                    class Foo implements \Serializable {
                        public function unserialize(string $serialized) {}
                        public function serialize() {}
                    }',
                'error_message' => 'InvalidReturnType',
            ],

Or moved into valid cases :)

@orklah
Copy link
Collaborator

orklah commented Sep 6, 2021

@SCIF see https://3v4l.org/ZGMmf

Psalm support PHP 7.1 and above. What must be done is add a version number to this test so it's only checked for a specific version

@weirdan
Copy link
Collaborator

weirdan commented Sep 6, 2021

@SCIF 7.4 is still in active support phase, so it's too soon to call that outdated. It's just version-dependent - valid for 8.0+, but invalid for <8.0.

@orklah
Copy link
Collaborator

orklah commented Sep 6, 2021

however this probably means we need to have stubs that depend on the php version. We already have a Php80.phpstub, but I think Psalm will fail if we have a class in two different stub, so we would need to have a Php7(1|2|3|4).phpstub to contain old signatures

@SCIF
Copy link
Contributor Author

SCIF commented Sep 16, 2021

The test crashed is actually incorrect. Docs

@orklah
Copy link
Collaborator

orklah commented Sep 16, 2021

That seem correct, you can get rid of the test

@SCIF
Copy link
Contributor Author

SCIF commented Sep 16, 2021

Done :)

@orklah orklah merged commit 7328456 into vimeo:master Sep 20, 2021
@orklah
Copy link
Collaborator

orklah commented Sep 20, 2021

Thanks!

@weirdan weirdan added the release:fix The PR will be included in 'Fixes' section of the release notes label Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants