larastan: Custom eloquent collection not detected

  • Larastan Version: v2.9.4
  • Laravel Version: v10.48.8

Description

If a model uses a custom collection (through newCollection()), the new collection type isn’t detected. This presents in 2 ways:

  1. Correct collection, but unknown element type User:
$userQuery->chunk(1000, function(CustomCollection $users) {
    \PHPStan\dumpType($users); << Dumped type: CustomCollection
    foreach ($users as $user) {
        var_dump($user->fullname); << Access to an undefined property Illuminate\Database\Eloquent\Model::$fullname
    }
});

If I omit the CustomCollection type in the chunk() callback, PHPStan says Dumped type: Illuminate\Database\Eloquent\Collection<int, App\Models\User> which is also half-correct, but the other half (wrong Collection class, right element type).

  1. Incorrect collection, but known element type User:
$users = $query->get();
\PHPStan\dumpType($users); << Dumped type: CustomCollection
foreach ($users as $user) {
    \PHPStan\dumpType($user); << Dumped type: Illuminate\Database\Eloquent\Model
    var_dump($user->fullname); << Access to an undefined property Illuminate\Database\Eloquent\Model::$fullname
}

I feel like PHPStan or Larastan could be smarter and combine the two: it’s definitely a CustomCollection, because PHP makes sure, and static analysis says it contains App\Models\User. Combine it into a CustomCollection<App\Models\User>? 😆

Demo repo: https://github.com/rudiedirkx/larastan-demo

About this issue

  • Original URL
  • State: closed
  • Created 2 months ago
  • Comments: 67 (26 by maintainers)

Most upvoted comments

Ah, yes. You are right. I was thinking the opposite way for some reason 😅

image

I mapped a collection of EngineerSeal models to State models—an EloquentCollection is returned, not SupportCollection.

I don’t have a custom collection, but if this were then the custom collection would be returned

@rudiedirkx, open up a new issue with the relevant details and I’ll take a look

You should look into custom builders if you don’t want to use scopes

However, I see no reason to have the method indexQuery just wrap query…just use ::query().

That was a simplified example =) There’s a lot of logic in there, with arguments. I could move it into a scope, and Larastan would probably understand that, but I like to keep the method names literal (no scope* magic) when possible. I’ll just add the @return.

You still need generics on the newCollection:

/**
 * @param array<int, static> 
 * @return ModelCollection<int, static>
 */
public function newCollection(array $models = []): ModelCollection
{
    return new ModelCollection($models);
}

Also, for flexibility (and to use composition rather than inheritance), I would recommend using the following trait instead of sticking this method on your base model—this will allow you to use other custom collections and even apply it on models that don’t extend your base model

/**
 * @template TCollection of EloquentCollection
 * @mixin Model
 */
trait HasCollection
{
    // Must add the following property to the model
    // protected static string $collection;

    /**
     * @param array<int, static> $models
     * @return TCollection
     */
    public function newCollection(array $models = []): EloquentCollection
    {
        return new static::$collection($models);
    }
}

Then to use:

class FooModel extends Model
{
    /** @use HasCollection<ModelCollection<int, static>> */
    use HasCollection;

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

Or if the custom collection is not generic:

class BarModel extends Model
{
    /** @use HasCollection<BarCollection> */
    use HasCollection;

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

This is a pattern I’ve been using for Builders and Factories to reduce the amount of code on the models

Everything looks pretty good to me! Larastan should be able to understand newCollection if all the generics are set up properly.

App\Base\ModelCollection&iterable<int, App\Models\Site> is an intersection type and occurs when there’s only one argument to Collection like Collection<Model>. If you ever see that, it usually means the generics weren’t defined properly.

How do you define newCollection on your base model?

No, you need to define the generics in your custom collection class.

Either:

/** @extends EloquentCollection<int, FooModel> */
class FooCollection extends EloquentCollection

if the collection class does not need to be generic itself.

Or

/**
 * @template TKey of array-key
 * @template TModel of FooModel // or Model
 *
 * @extends EloquentCollection<TKey, TModel>
 */
class FooCollection extends EloquentCollection

if the collection does need to be generic.

If the collection is generic itself, then you would need to define the generics where you use it:

/** @return FooCollection<int, FooModel> */