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] fix: Model newCollection generics; feat: add HasCollection trait #52171

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Jul 17, 2024

Hello!

This PR fixes some issues with the Model's newCollection generic (phpstan/phpstan#11354) ---it appears that due to covariant/invariant constraints the method must return only static models.

I also added a HasCollection trait similar to the Has{Builder,Factory} traits that help slim down the models:

// before
class Post extends Model
{
    /** @return PostCollection<array-key, static> */
    public function newCollection(array $models = []): PostCollection
    {
        return new PostCollection($models);
    }
}

// after
class Post extends Model
{
    /** @use HasCollection<PostCollection<array-key, static>> */
    use HasCollection;

    protected static string $collection = PostCollection::class;
}

Thanks!

@henzeb
Copy link
Contributor

henzeb commented Jul 18, 2024

Looking at your code, you add a trait, but then you aren't doing anything with the trait, it has zero effect, the newCollection is still in Model.

@calebdw
Copy link
Contributor Author

calebdw commented Jul 18, 2024

The trait is for use in user land when a custom collection class is used as it better helps with generics/autocomplete/ide support, however, I suppose we could add the trait to the base model like this:

https://phpstan.org/r/0c35c490-55a1-48fa-b407-d58ae52cb499

@henzeb
Copy link
Contributor

henzeb commented Jul 18, 2024

The trait is for use in user land when a custom collection class is used as it better helps with generics/autocomplete/ide support

if it is just for that, it doesn't slim down anything. I suggest either remove the trait or move generic logic into that trait. leaving Model as was.

edit: Given that you also need logic inside those Collections to enforce a certain type: look at packages like this, who already does, what this PR wants to do, for you: https://packagist.org/packages/henzeb/laravel-typed-collection

@calebdw
Copy link
Contributor Author

calebdw commented Jul 18, 2024

I've updated the Model to use the HasCollection trait, but I can always revert if this is not desired

@taylorotwell taylorotwell merged commit a488974 into laravel:11.x Jul 18, 2024
29 checks passed
@henzeb
Copy link
Contributor

henzeb commented Jul 18, 2024

The trait is still not doing anything except for extending the original newCollection method. The trait should not have to be part of Model by default. by default Model uses the default Collection class. But only when adding that trait, one should be able to set a specific Collection classname. No code changes in Model, just a new trait.

@calebdw
Copy link
Contributor Author

calebdw commented Jul 18, 2024

Thanks @taylorotwell!

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.

None yet

4 participants