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

[9.x] Uses generic collections on Eloquent\Model::class #38575

Closed
wants to merge 1 commit into from

Conversation

nunomaduro
Copy link
Member

Note: this pull request is waiting for a PHPStan release

This pull request makes Eloquent\Model::class to use generic collections (#38538). In other words, the goal is simply search for Collection, and add the information - regarding generics - to those annotations. Of course, the benefit is, at some point, have auto-completion/static analysis support on code like:

$users = User::where('active', 1)
    ->orderBy('name')
    ->take(10);

$users->first()->userMethod();

Also, in the process, and thanks the @mixin annotation on the Eloquent\Model::class, without the IDE Helper, PHPStorm is able to understand a little bit more of the Model Builder class:

Screenshot 2021-08-27 at 19 14 31

Finally, at this point, is worth waiting for having PHPStorm understand generics before moving forward. If we go deeper on eloquent - as we did with collections (fully type arguments, etc ) - we actually take the risk of losing some auto-completion.

@nunomaduro
Copy link
Member Author

nunomaduro commented Sep 6, 2021

@taylorotwell After giving some thought into this pull request, and while can be attempting to add this pull request to the framework right now, I would rather wait for a larger adoption by editors, before implementing this on Eloquent.

The reasoning behind this decision is that to make a clean work — like the one we have done on generic collections — we would have to remove some existing IDE support on Eloquent, here is one example:

    /**
-    * @return \Illuminate\Database\Eloquent\Model|\Illuminate\Database\Eloquent\Collection|static[]|static|null
+    * @return \Illuminate\Database\Eloquent\Collection<int, TModel>|TModel|null
     */
    public function find($id, $columns = ['*']) {

By doing this change, static analysis tools would be happy, yet, existing IDE support, would lose auto-completion on User::find(1)->save(); or foreach (User::find(1) as $user) { $user->save(); }.

That been said, generics collection is already a big step on this mission, when people can already use them in their own code. Yet, to have them on Eloquent, is worth to wait a little bit longer so we can have a smooth change. So, in my opinion, lets close this pull request, and revisit it once existing editors are able to fully follow us on this.

@nunomaduro nunomaduro closed this Sep 6, 2021
@driesvints driesvints deleted the feat/generic-builder branch September 27, 2021 09:17
@morrislaptop
Copy link
Contributor

Hey @nunomaduro, found this from a twitter thread - with the recent support for generics from phpstorm should this be revisited?

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