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

[11.x] Enhance PHPDoc for Eloquent Relations to support precise subclasses #52775

Conversation

kayw-geek
Copy link
Contributor

Description:

Currently, all PHPDoc annotations related to Eloquent relations require the native typehint to be declared as \Illuminate\Database\Eloquent\Relations\Relation class. This forces users who originally declared more specific relation subclass typehint to change them to the general \Illuminate\Database\Eloquent\Relations\Relation class.

This PR introduces method-scope covariant generics to resolve this issue, allowing PHPDoc to correctly support more specific relation subclass typehint without reverting to the general Relation class typehint.

This change also avoids the usage of * to pass meaningless generic parameters for the Relation class, thereby improving the readability and accuracy of the code.

@calebdw
Thank you for reviewing this PR. I look forward to your feedback. If there are any further suggestions or concerns, please feel free to reach out.

@kayw-geek kayw-geek force-pushed the feature/phpdoc-relation-subclass-support branch from 6360f01 to 2364fc4 Compare September 13, 2024 02:47
@kayw-geek kayw-geek force-pushed the feature/phpdoc-relation-subclass-support branch from 2364fc4 to 24e4e8a Compare September 13, 2024 02:51
@calebdw
Copy link
Contributor

calebdw commented Sep 13, 2024

@kayw-geek,

Unfortunately, this will not work either: https://phpstan.org/r/f76f9b3b-f625-483c-9f18-b7de88b80477

  • @template-covariant is only allowed above classes and interfaces, not individual methods
  • having the method be generic means that all the closures must have the same type (which is not the case), it fails as soon as you introduce two different relations

This change also avoids the usage of * to pass meaningless generic parameters for the Relation class, thereby improving the readability and accuracy of the code.

You are mistaken on this, the wildcard (*) does not allow you to "pass meaningless generic parameters for the Relation class"---the types are separately checked by the template constraints declared on the Relation class (i.e., TRelatedModel of Model). Trying to pass a Relation with "meaningless generic parameters" will fail.

Higher PHPStan levels require that all inner generic types be defined, the wildcards in this case simply say: "I will accept any valid Relation and I don't care what the inner types are". The following examples are functionally equivalent, which would you rather type out everywhere?

/** \Illuminate\Database\Eloquent\Relations\Relation<*,*,*> */
// or
/** \Illuminate\Database\Eloquent\Relations\Relation<\Illuminate\Database\Eloquent\Model,\Illuminate\Database\Eloquent\Model,mixed> */

@kayw-geek
Copy link
Contributor Author

Ah, I passed the test in my project. Maybe the wrong phpstan defined in the vendor will not be exposed in my project. sorry.

@kayw-geek kayw-geek closed this Sep 13, 2024
@calebdw
Copy link
Contributor

calebdw commented Sep 13, 2024

Ah, I passed the test in my project.

That is for a couple of reasons:

  • the phpstan analysis of the actual source code is too low (only level 1)
  • your type tests really aren't testing anything, and had you passed two different relations they should have failed

The following type assertions aren't really doing much because they are guaranteed to be the type specified in the closure (HasOne $query):

function (HasOne $query): void {
    assertType('Illuminate\Database\Eloquent\Relations\HasOne', $query);
}

What is more important to test is that PHPStan can infer what is the type inside the closure without the closure type:

function ($query): void {
    assertType('Illuminate\Database\Eloquent\Relations\Relation', $query);
}

PHPStan is currently having issues with closures inside arrays, which is why all of the type assertions are commented out

@kayw-geek
Copy link
Contributor Author

Thank you for giving such a detailed answer. You are an excellent developer.

@calebdw
Copy link
Contributor

calebdw commented Sep 13, 2024

Thank you for giving such a detailed answer.

You're welcome! I'm happy to help explain any areas of confusion. Granted, I could be wrong about some things, but this is my understand from all the time I've spent working with Larastan/PHPStan.

I understand where you're coming from by wanting to type hint HasOne instead of Relation, but if there was a better way to do this that I was aware of then I would have done it. Feel free to open an issue at phpstan to see if such a thing is possible and type safe.

At the end of the day though I'm not sure it matters to much though because most of the methods you call on the Relation object are Builder methods that will be forwarded to the underlying query. I can't really think of specific Relation methods that you would need to call

You are an excellent developer.

Thank you! But you are too kind and give me too much credit. I just do what I can :)

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.

2 participants