framework

framework copied to clipboard

Hello!

I've been contributing a bunch to Larastan, and have been wanting to integrate the relation generics into the framework for a while now. Generics provide better auto-completion and intellisense in the ide without having to rely on Larastan to add generics to the classes through the use of stubs. Having generics in the framework also makes it easier for third party packages to define the inner types on their custom relations.

There is another PR (#51681) open to add generics to Relations, but this offers a more complete and better tested implementation. I've gone ahead and implemented @nagmat84's fixes and notation from his PR (https://github.com/larastan/larastan/pull/1285) to fix the Larastan stubs. Please see that PR for an extensive write up on the rationale behind the changes.

Examples:

class User extends Model
{
    /** @return HasOne<Address, $this> */
    public function address(): HasOne
    {
        return $this->hasOne(Address::class);
    }

    /** @return HasMany<Post, $this> */
    public function posts(): HasMany
    {
        return $this->hasMany(Post::class);
    }

    /** @return BelongsToMany<Role, $this> */
    public function roles(): BelongsToMany
    {
        return $this->belongsToMany(Role::class);
    }

    /** @return HasOne<Mechanic, $this> */
    public function mechanic(): HasOne
    {
        return $this->hasOne(Mechanic::class);
    }

    /** @return HasOneThrough<Car, Mechanic, $this> */
    public function car(): HasOneThrough
    {
        return $this->hasOneThrough(Car::class, Mechanic::class);
    }

    /** @return HasManyThrough<Part, Mechanic, $this> */
    public function parts(): HasManyThrough
    {
        return $this->hasManyThrough(Part::class, Mechanic::class);
    }
}

New HasOneOrManyThrough Relation

I created an abstract HasOneOrManyThrough class that the HasOneThrough/HasManyThrough classes extend (similar to the other OneOrMany abstract classes). This was necessary to allow the HasManyThrough class to properly bind the TResult generic to a collection instead of leaving it unbound for HasOneThrough class to bind to a Model.

HasBuilder Trait

I created a HasBuilder trait that reduces boilerplate when using custom Builders---while they are not for everyone, custom Builders do work better with static analysis / auto-completion over scopes and they help slim down the models. The trait helps IDEs better resolve the correct classes when they are used.

For example:

// before
class Post extends Model
{
    /** @return CommonBuilder<static> */
    public static function query(): CommonBuilder
    {
        return parent::query();
    }

    /**
     * @param  \Illuminate\Database\Query\Builder  $query
     * @return CommonBuilder<*>
     */
    public function newEloquentBuilder($query): CommonBuilder
    {
        return new CommonBuilder($query);
    }
}

// after
class Post extends Model
{
    /** @use HasBuilder<CommonBuilder<static>> */
    use HasBuilder;

    protected static string $builder = CommonBuilder::class;
}

Thanks!

I've been waiting for this for a long time!

This is a must-have if you want to run larastan (phpstan) on level 7 or above and have re-usable scopes in a Trait for example... Currently there's no way to use scopes from a Trait (eg. morphable logic) and type them as Builder - you need to default back to object type which is not allowed on level 7 and above

Hey @calebdw - thanks for looking into this. What made you send it to master instead of 11.x?

@taylorotwell, sure thing!

I targeted master because while this shouldn't have any runtime BCs, this could cause other folks' CI to fail depending on the PHPStan level used. That being said, if this is acceptable then I can certainly update to target 11.x.

Note that I also created a HasBuilder trait that reduces boilerplate when using custom Builders---while they are not for everyone, custom Builders do work better with static analysis / auto-completion over scopes and they help slim down the models. The trait helps IDEs better resolve the correct classes when they are used.

For example:

// before
class Post extends Model
{
    /** @return CommonBuilder<static> */
    public static function query(): CommonBuilder
    {
        return parent::query();
    }

    /**
     * @param  \Illuminate\Database\Query\Builder  $query
     * @return CommonBuilder<*>
     */
    public function newEloquentBuilder($query): CommonBuilder
    {
        return new CommonBuilder($query);
    }
}

// after
class Post extends Model
{
    /** @use HasBuilder<CommonBuilder<static>> */
    use HasBuilder;

    protected static string $builder = CommonBuilder::class;
}

However, if this is not desired then I can just pop the HasBuilder commit off.

Let's just target this to 11.x.

@taylorotwell, yes sir---done!

We will need to wait for @nunomaduro to take a look at this. Would appreciate other community feedback in the meantime.

Just ran this on one of our web apps to check if it works, and it seems to work fine, reporting a few additional things that were statically ambiguous.

Was hoping this change might actually help in solving https://github.com/larastan/larastan/issues/1538, but so far it didn't, but maybe that's because there's still some stubs from Larastan taking precedence over changes here? If so, there's probably a parallel MR required to remove those parts of the stubs there that might otherwise conflict with things in here?

@jnoordsij, yes changes to Larastan will be needed which I plan on implementing

Thanks for your work on this!

Thank you!

Thanks @calebdw 👍

@calebdw thanks 🚀 I can improve and remove useless /** @var ... */ from my code 🔥

but not everything works correctly, for example:

my fault, I read and found your another pr for doing this: https://github.com/laravel/framework/pull/51681

@siarheipashkevich, it's always good to remove overrides!

If you're using Larastan then I'm working on a PR (https://github.com/larastan/larastan/pull/1990) to add support for the new generics. If you're not using Larastan then there are some limitations (particularly when you start getting into method forwarding) that can't be solved with PHPDocs alone. However, I have tests for your particular example so the issue is likely in your relation phpdocs

https://github.com/laravel/framework/blob/68e88bbcee1e9b0b02ca267916ee25f45ab3935c/types/Database/Eloquent/Relations.php#L38

thats what we have been waiting for long time thanks @taylorotwell @calebdw

Are these changes cause of this? Now every call of relation method is highlighted by PhpStorm.

@decadence This kind of seems like a PHPStorm Problem, because it tries to search class-string within a namespace instead of using it as a literal. No clue why though.