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] feat: add generics to Eloquent Builder and Relations #51851

Draft
wants to merge 4 commits into
base: 11.x
Choose a base branch
from

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Jun 20, 2024

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 (larastan/larastan#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!

@calebdw calebdw force-pushed the relation_generics branch 2 times, most recently from 6a0123b to 90b5bd0 Compare June 20, 2024 06:10
@rcerljenko
Copy link

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

@taylorotwell
Copy link
Member

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

@calebdw
Copy link
Contributor Author

calebdw commented Jun 20, 2024

@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.

@taylorotwell
Copy link
Member

Let's just target this to 11.x.

@calebdw calebdw changed the base branch from master to 11.x June 21, 2024 00:52
@calebdw calebdw changed the title [12.x] feat: add generics to Eloquent Builder and Relations [11.x] feat: add generics to Eloquent Builder and Relations Jun 21, 2024
@calebdw
Copy link
Contributor Author

calebdw commented Jun 21, 2024

@taylorotwell, yes sir---done!

@calebdw calebdw force-pushed the relation_generics branch 3 times, most recently from 0a3f29e to 22e2b14 Compare June 21, 2024 05:36
@taylorotwell
Copy link
Member

taylorotwell commented Jun 21, 2024

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

@taylorotwell taylorotwell marked this pull request as draft June 21, 2024 15:59
@jnoordsij
Copy link
Contributor

jnoordsij commented Jun 28, 2024

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 larastan/larastan#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?

@calebdw
Copy link
Contributor Author

calebdw commented Jun 28, 2024

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

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

8 participants