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

Handle magic calls on model and query builder correctly #325

Merged
merged 26 commits into from
Oct 21, 2019
Merged

Handle magic calls on model and query builder correctly #325

merged 26 commits into from
Oct 21, 2019

Conversation

canvural
Copy link
Collaborator

@canvural canvural commented Oct 10, 2019

Ok. This turned out to be a little bigger PR than I imagined 😅 But the outcome is good! I tested it on my big project. And I could go to level 4 from level 3 without changing anything.

List of changes:

  • Model class' __call method is mimicked. If it's not increment or decrement calls are forwarded to \Illuminate\Database\Eloquent\Builder
  • \Illuminate\Database\Eloquent\Builder __call method is mimicked. With one exception. Local macros are not supported. Otherwise, all unknown calls are forwarded to \Illuminate\Database\Query\Builder class. But return type is still \Illuminate\Database\Eloquent\Builder
  • Added support for calling get on the query builder. This will return Collection class and elements of the collection correctly typehinted with the model.
  • Return type of query builder methods is fixed. Previously User::where('foo', 'bar') was returning the model itself. See Wrong tests and possible wrong implementation #324 This is fixed now
  • Return type of model local scopes is correctly handled now.
  • Added correct return types for model retrieval and creation methods. Ex. find, firstOrCreate etc.
  • Added more test cases for everything.

There might be an increased number of errors after these changes. But this is because there were false negatives before. For example Larastan would think User:whereFoo('bar')->someRelation() is a correct code and returning model instance. But that example is not a valid Laravel code in reality. So I guess any of this can't be considered as breaking change. And all the existing (which I didn't change) tests are passing, so existing functionality is preserved.

I'm open to any comments and questions. About implementation or other things.

@canvural canvural changed the title Add forwards calls Handle magic calls on model and query builder correctly Oct 10, 2019
@szepeviktor
Copy link
Collaborator

Too big to fail.

Copy link
Collaborator

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

This pull request is awesome. Congrats buddy! And thank you again for your work.

src/Methods/ModelForwardsCallsExtension.php Outdated Show resolved Hide resolved
@nunomaduro nunomaduro self-requested a review October 11, 2019 11:12
@canvural
Copy link
Collaborator Author

@nunomaduro Can we merge this and make a new 0.4.2 release? Or do you want this in a new release? 0.5 maybe ?

@szepeviktor
Copy link
Collaborator

@canvural Please do release v0.4.2

@canvural
Copy link
Collaborator Author

@nunomaduro Is there anything left to do here? Can we merge?

Copy link
Collaborator

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

LGTM

@canvural
Copy link
Collaborator Author

Ok, so it's up to you. We can't merge because of the continuous-integration/styleci/push Expected status. 🙄

@bobbybouwmann
Copy link

@nunomaduro Can we force merge this, since we're only waiting for the style ci?

@nunomaduro nunomaduro merged commit 47ad926 into larastan:master Oct 21, 2019
@nunomaduro
Copy link
Collaborator

Done.

@bobbybouwmann
Copy link

@nunomaduro Do you have an ETA for the release? :D

@canvural
Copy link
Collaborator Author

canvural commented Oct 21, 2019

I was thinking maybe we can test it a little, with dev-master in composer, and then make a release if everything seems fine.

But I can make the release 0.4.3, if it's ok with @nunomaduro

@canvural canvural deleted the add-forwards-calls branch October 21, 2019 13:59
@spaceemotion
Copy link

I will try the changes as soon as I am able (still today). Will report back in the issues once I got around to it.

@bobbybouwmann
Copy link

Thanks for the awesome work guys!

@nunomaduro
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-package enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants