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

Add custom collection support for get and all methods #903

Merged
merged 6 commits into from
Apr 19, 2020
Merged

Add custom collection support for get and all methods #903

merged 6 commits into from
Apr 19, 2020

Conversation

dmason30
Copy link
Contributor

@dmason30 dmason30 commented Mar 31, 2020

Related to: #273 #171

Currently custom collections are only provided code completion when accessed via a relation that returns a Collection (HasMany, BelongsToMany etc.).

This PR adds code completion on the get and all methods. Tests included.

public function newCollection(array $models = [])
{
     return new CustomCollection($models);
}

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

After testing this PR on real codebase I've the following feedback:

  • The all method without a custom collection is already correctly resolved due to laravels phpdoc on \Illuminate\Database\Eloquent\Model::all:
       * @return \Illuminate\Database\Eloquent\Collection|static[]
    The PR in the current state however adds it all the time to models although redundant. If would only be necessary if there truly is a newCollection method returning a different collection
  • The get method, though technically not exactly the same, however behaves the same when using the ide-helper generate command
    • in which case Model::get resolves to the definition within the _ide_helper.php file also correctly:
               * @return \Illuminate\Database\Eloquent\Collection|static[] 

Although I'm aware it's up to everyone which parts of the package to use, you get only most of out of it when combining things.

Therefore my suggestion, for both new phpdoc methods: only generate them if the returned class is not \Illuminate\Database\Eloquent\Collection

This cuts down a lot of noise on existing models which have no such customization.

What do you think about that?

@dmason30
Copy link
Contributor Author

dmason30 commented Mar 31, 2020

@mfn Thanks for the feedback. I have added the change so that it only adds these when using a custom collection.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Nice that you also used static[], totally makes sense.

Also tested in on a real project, works as expected and I'm really glad it does not generate the "unnecessary noise" for those not actually using custom collections!

IMHO this is good to merge @barryvdh !

Daniel Mason and others added 2 commits April 1, 2020 13:36
@dmason30
Copy link
Contributor Author

@mfn Is this likely to get merged and released soon? I am using a fork atm and am happy to carry on doing so but there are a lot of other nice PRs on here 👍

@mfn
Copy link
Collaborator

mfn commented Apr 18, 2020

Is this likely to get merged and released soon?

I can't say, I don't have more stakes here than you 🤷‍♀️

@barryvdh barryvdh merged commit 6a27c5d into barryvdh:master Apr 19, 2020
@barryvdh
Copy link
Owner

Thanks!

@dmason30 dmason30 deleted the custom-collection-support branch April 20, 2020 03:45
@dmason30
Copy link
Contributor Author

dmason30 commented Apr 22, 2020

@mfn @barryvdh Just tried in a few applications and we get custom collection code completion on

Foo::get()
Foo::all()

Also if you build a query using query then you do get code completion:

Foo::query()->where('bar', 'baz')->get() // completes to custom collection

But if I build a query directly from the model there is no completion even though it does return the custom collection instance 😿.

Foo::where('bar', 'baz')->get() // completes to Builder[]|Collection 

Can you see an obvious way to solve this? I am willing to PR another go but I can't see how this can be done without adding docs for all the builder methods in the Illuminate\Database\Eloquent\Model with the already added @mixin.

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.

3 participants