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

feat: support relation withDefault #1833

Closed
wants to merge 2 commits into from
Closed

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Feb 9, 2024

  • Added or updated tests

Closes #1669

Changes

This checks every relation to see if withDefault is called, and if so, returns the relation type without the null union.

Note, this is now (potentially) parsing the file twice for each relation: once for the related model name and then again to check for the default method. It might be worth trying to only parse once.

Thanks!

@calebdw calebdw force-pushed the with_default branch 2 times, most recently from f7070f5 to 8322102 Compare February 27, 2024 15:58
@canvural
Copy link
Collaborator

Hi,

While I understand the issue this is solving. I don't think this is the way to go.

We parse the model files, only if the relationship method does not provide any generic return type. This is a special case we take in order to make the developer's life easier. They don't need to provide the generic return type for relation methods. But with this change we'd definitely parse the file for every call now.

Plus this goes against the philosophy of PHPStan itself. It does not descend down into the method bodies it analyses because of performance reasons.

We might solve this in version 3 though. By adjusting the generics and using similar techniques that we use in factories.

Hope it makes sense. Thanks!

@canvural canvural closed this Apr 14, 2024
@calebdw
Copy link
Contributor Author

calebdw commented Apr 14, 2024

@canvural, I believe that the vast majority of developers do not add the generic type hints to their relationships (there was an issue last week where someone was strongly opposed to just adding the php return type, much less the generic!)---so in reality I believe that the models are already being parsed several times over.

I have been thinking about this, I think we need to implement caching here so that every method / model is parsed at most once (similar to parsing the migrations). This would also allow us to support other things in a single pass (withDefault, as, withPivot, etc)---I think this is also integral to solving #1308 & #1774.

@calebdw
Copy link
Contributor Author

calebdw commented Apr 14, 2024

@canvural, you've also mentioned v3 a few times. Can you create a 3.x branch (or master) so we can start making PRs against it? It would also be helpful to come up with a roadmap to v3 that we can start working towards.

@canvural
Copy link
Collaborator

I believe that the vast majority of developers do not add the generic type hints to their relationships (there was an issue last week where someone was strongly opposed to just adding the php return type, much less the generic!)

Then they should not even use a tool that's whole purpose is checking types 🙂

you've also mentioned v3 a few times. Can you create a 3.x branch (or master) so we can start making PRs against it? It would also be helpful to come up with a roadmap to v3 that we can start working towards.

I'd rather make an issue first where we can gather the ideas for v3. I'll try to open it this week.

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.

Default Models
3 participants