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

Add psalm specific generic return type for deserialize #1091

Merged
merged 1 commit into from Jun 3, 2019

Conversation

@bdsl
Copy link
Contributor

@bdsl bdsl commented Jun 3, 2019

See https://psalm.dev/ and
https://medium.com/vimeo-engineering-blog/uncovering-php-bugs-with-template-a4ca46eb9aeb

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #...
License MIT
@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Jun 3, 2019

These annotations tell the Psalm static analyser that the object returned by deserialize will be an instance of the class named in the $type parameter. Psalm can use that to find errors, e.g. calling a function that doesn't exist in that class, and also to provide autocompletion suggestions.

@bdsl bdsl force-pushed the templated-return branch from 14b8fd2 to e9e8fa2 Jun 3, 2019
@goetas
Copy link
Collaborator

@goetas goetas commented Jun 3, 2019

Im fine with this, but is there a way to test it?

@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Jun 3, 2019

@goetas Probably if you want to have this work reliably you'd need need to add psalm to the build pipeline for serializer, firstly to check that it doesn't error when parsing this docblock, and then also to make sure that it infers the type correctly when a caller uses this method.

For now you can see how this allows psalm to detect a spelling mistake when calling a function on the DTO at https://psalm.dev/r/1f725d788b

@goetas goetas merged commit 48be7ea into schmittjoh:master Jun 3, 2019
1 check passed
@goetas
Copy link
Collaborator

@goetas goetas commented Jun 3, 2019

thanks

@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Jun 3, 2019

You're welcome, thanks for the quick response. Do you have any plan for when this would be in a release? I don't know if there are any set release dates or roadmaps for this project.

@bdsl bdsl deleted the templated-return branch Jun 3, 2019
@goetas
Copy link
Collaborator

@goetas goetas commented Jun 3, 2019

this project does not have "time scheduled" releases. Generally when I have some time I create a changelog and do the release.

@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Jun 3, 2019

Thanks @goetas . Let me know if you and other maintainers would like a PR to add psalm to the travis build. Psalm has a baselining feature so any issues it finds in when first set up don't have to be immediately fixed.

@goetas
Copy link
Collaborator

@goetas goetas commented Jun 4, 2019

Having some basic Psalm integration into the serializer would be very nice. I guess is not any easy task, but would be happy to see a PR with that

@bdsl bdsl mentioned this pull request Jun 4, 2019
@carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Dec 18, 2019

@bdsl How are we supposed to annotate the classes that we pass to the deserialize method from now on?

@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Dec 18, 2019

@carusogabriel This PR didn't change anything about how you should use deserialize. It just means that when you write something like $dto = $serializer->deserialize($json, ExampleDto::class, 'json') Psalm will know that $dto is an ExampleDto object. You can take a look at https://psalm.dev/r/1f725d788b for an illustration.

I'm not sure if that answers your question.

@carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Dec 18, 2019

I'm getting some PHPStan 0.12 errors, like:

  • Unable to resolve the template type T in call to method JMS\Serializer\SerializerInterface::deserialize()
  • Parameter #2 $type of method JMS\Serializer\SerializerInterface::deserialize() expects class-string<mixed>, string given.

@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Dec 18, 2019

@carusogabriel hmm, probably best to raise a new issue for that, either here or at https://github.com/phpstan/phpstan/issues , with a minimal reproducing example if possible. I haven't actually used PHPStan myself yet.

@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Dec 20, 2019

@carusogabriel How did you get on? Will you make a new issue report?

@carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Dec 20, 2019

When I figure out what's happening in how to reproduce, yes, I'll report an issue here 👍

@bdsl
Copy link
Contributor Author

@bdsl bdsl commented Dec 20, 2019

Ok, feel free to tag me in when you do - I might have some time to look into how to fix it. Worth linking to this PR as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants