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

feature:generic resources #470

Merged
merged 1 commit into from Feb 21, 2020
Merged

Conversation

mr-feek
Copy link
Contributor

@mr-feek mr-feek commented Feb 19, 2020

resolves #437

I'm opening this up now to get feedback on if I am implementing correctly. I would appreciate some guidance as this is my first time working with generics and understanding what phpstan does behind the scenes.

Open Questions

  • How do I implement getMethod and getProperty. Off the top of my head, I would think return $modelType->getMethod($methodName); however this requires a \PHPStan\Reflection\ClassMemberAccessAnswerer parameter, and I'm not sure what that is or how to resolve it.
  • How do I test my extension correctly catches failing statements? IE I want to ensure $resource->foo() is caught as an error.

Improvements

  • NOT POSSIBLE: Is there a way we can infer the type passed to a Resource without having to declare
/**
 * @extends JsonResource<User>
 */

At the top of resources? This is not ideal, and can hopefully be circumvented. I'm not sure how.

  • Along with the above, ideally I would think $templateTypeMap = $classReflection->getActiveTemplateTypeMap(); would resolve the model type, rather than having to fetch it via the first parent...

@canvural
Copy link
Collaborator

Hi,

First, you can make sure $modelType is instance of ObjectType. If not you can return false. then you can use getClassReflection method to get the class reflection. Then you can use the hasMethod, getMethod etc. method on the reflection.

@extends JsonResource<User> doc block is required. All classes that extends a generic class should specify they are also generic by using this. You can read this article to have more knowledge of generics.

$classReflection->getActiveTemplateTypeMap(); should work. No need to reach to the parent. Why doesn't it work for you?

@mr-feek
Copy link
Contributor Author

mr-feek commented Feb 19, 2020

then you can use getClassReflection method to get the class reflection. Then you can use the hasMethod, getMethod etc. method on the reflection.

I see!! Thank you!

@mr-feek
Copy link
Contributor Author

mr-feek commented Feb 19, 2020

I believe I am now using getMethod and getProperty properly now, passing an OutOfClassScope instance

@mr-feek mr-feek marked this pull request as ready for review February 19, 2020 23:24
@mr-feek mr-feek changed the title [WIP] feature:generic resources feature:generic resources Feb 19, 2020
@mr-feek
Copy link
Contributor Author

mr-feek commented Feb 19, 2020

Although I haven't figured out how to solve

Along with the above, ideally I would think $templateTypeMap = $classReflection->getActiveTemplateTypeMap(); would resolve the model type, rather than having to fetch it via the first parent...`

The tests are passing so I'm going to open this up for review.

@canvural canvural self-requested a review February 20, 2020 08:24
README.md Outdated Show resolved Hide resolved
src/Properties/JsonResourceExtension.php Show resolved Hide resolved
stubs/JsonResource.stub Outdated Show resolved Hide resolved
tests/Features/Resources/ResourceTest.php Outdated Show resolved Hide resolved
tests/Features/Resources/ResourceTest.php Outdated Show resolved Hide resolved
@mr-feek
Copy link
Contributor Author

mr-feek commented Feb 20, 2020

@canvural thank you for the review, feedback, and helping me learn about the workings of phpstan and generics.

Would you mind giving this another review please?

@canvural canvural self-requested a review February 21, 2020 07:57
Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

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

Looks good to merge. One last thing. Can you add this change to CHANGELOG under Unreleased?

@mr-feek
Copy link
Contributor Author

mr-feek commented Feb 21, 2020

done @canvural :)

@canvural canvural merged commit 8f22d05 into larastan:master Feb 21, 2020
@mr-feek mr-feek deleted the generic-resources branch February 26, 2020 22:22
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.

Support for Eloquent Resources
2 participants