Custom builder for relation methods

[BertvanHoekelen]

• Larastan Version: 0.5
• --level used: 5

Description

Calling custom builder methods on a relationship works in Laravel but larastan shows the following error: Method Illuminate\Database\Eloquent\Model::uncategorized() invoked with 1 parameters, 0 required..

Laravel code where the issue was found for example:

$uncategorizedTopics = $user->topics()->uncategorized(true)->get();

I would love to contribute but I have no idea where to start.
My idea is that whenever a Relation class is returned from a function it should check the class given in the relationship for a custom builder and return an IntersectionType of the relations class (e.g. HasMany) and the custom builder.

Changing the test EloquentBuilderExtension to the following should test it I believe.

<?php

declare(strict_types=1);

namespace Tests\Features\ReturnTypes;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasMany;

class EloquentBuilderExtension
{
    public function testModelScopeReturnsBuilder(): Builder
    {
        return TestScopesModel::query()
            ->foo('piet');
    }

    public function testCustomBuilderMethodReturnsBuilder(): CustomBuilder
    {
        return TestScopesModel::query()
            ->type('piet');
    }

    public function testRelationReturnsCustomBuilder(): CustomBuilder
    {
        return TestScopesModel::testScopes();
    }
}

class TestScopesModel extends Model
{
    public function scopeFoo(string $foo) : Builder
    {
        return $this->where(['foo' => $foo]);
    }

    public function testScopes(): HasMany
    {
        return $this->hasMany(TestScopesModel::class);
    }

    /**
     * @param \Illuminate\Database\Query\Builder $query
     *
     * @return \Tests\Features\ReturnTypes\CustomBuilder
     */
    public function newEloquentBuilder($query)
    {
        return new CustomBuilder($query);
    }
}

class CustomBuilder extends Builder
{
    public function type(string $type): void
    {
        $this->where(['type' => $type]);
    }
}

[szepeviktor]

Very nice bug report.

[canvural]

Hi,

Thanks for the report!

The error you got is a little weird. It thinks uncategorized method is on Model class. Can you give a full example of the code?

If you want to fix this you can do it in ModelScopeAfterRelations file. Maybe you can rename this class in the process.

You should check the return type of newEloquentBuilder on the $classReflection and if that return type does have $passable->getMethodName() method. newEloquentBuilder will either return EloquentBuilder or the custom builder.

You should also check the magic __call method of the Relation class. There you can see a conditional. It's either returning the builder type or the relation. You should mimic that.

[BertvanHoekelen]

Hey,

Thanks for the reply! The error is definitely weird, but makes sense reading the ModelScopeAfterRelations file. I will try to fix it once I have some spare time!

A more complete example:

<?php

declare(strict_types=1);

namespace Tests\Unit;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Tests\TestCase;

class User extends Model
{
    public function topics(): HasMany
    {
        return $this->hasMany(Topic::class);
    }
}

class Topic extends Model
{
    public function newEloquentBuilder($query): TopicBuilder
    {
        return new TopicBuilder($query);
    }
}

class TopicBuilder extends Builder
{
    public function category(string $category): self
    {
        return $this->where('category', $category);
    }
}

class LaraStanTest extends TestCase
{
    public function test_larastan(): void
    {
        $user = new User();

        $user->topics()->category('Foo')->get();
    }
}

Results in:

------ ---------------------------------------------------------------------- 
  Line   StanTest.php                                                          
 ------ ---------------------------------------------------------------------- 
  42     Method Illuminate\Database\Eloquent\Model::category() invoked with 1  
         parameter, 0 required.                                                
 ------ ---------------------------------------------------------------------- 

[BertvanHoekelen]

Hmm, when looking into it, it seems like ModelScopeAfterRelations is not working correctly. It just forwards the method calls to Model and not to original class.

If I change testScopeAfterRelation to:

public function testScopeAfterRelation() : HasOne
{
    return $this->hasOne(User::class)->active('THIS_IS_DIFFERENT');
}

And the active scope to:

public function scopeActive(Builder $query, string $newParam): Builder
{
    return $query->where('active', 1);
}

I get a similar error as mentioned above Method Illuminate\Database\Eloquent\Model::active() invoked with 1 parameter, 0 required.

Expected
It should call \App\User::scopeActive with the $builderClass and THIS_IS_DIFFERENT as parameters. Or it should call \App\User::active and then it will be handled by EloquentBuilderExtension or ModelScopes I think.

I tried to get access to the User class from within ModelScopeAfterRelations but I have no idea how I can do that.

[canvural]

It's because of this line This needs to be changed.

I tried to get access to the User class from within ModelScopeAfterRelations but I have no idea how I can do that.

Yeah, you are right. I guess there is no way to get the model here. Maybe I can solve this later.

But I think we still have some issues like calling the query builder methods on the relation. For example $user->accounts()->where('foo', 'bar') should return the query builder and not the relation. Will you still be interested to fix this?

[BertvanHoekelen]

Let me know if I can be of some help testing something in a live project.

As for returning the query builder when using $user->accounts()->where('foo', 'bar'). Happy to help, if you can give me some pointers in which files I can find those implementations :)

[canvural]

It's the same file.

We should mimic the magic __call method from the Relation class. Basically you should check if Eloquent\Builder has $passable->getMethodName() method. If it's not there you should check Query\Builder.

Then you should get the method reflection for that, and check the return type. If it includes any Builder class, then the return type for the original method should be the relation. If not, we should use the return type we just found as the return type for the original method. This is because this code in Relation class

if ($result === $this->query) {
    return $this;
}

return $result;

[canvural]

Hi,

Can you test this again against the dev-master?

[BertvanHoekelen]

Hi @canvural,

I have tested it in my project. It's not complaining anymore but it's also not checking types or argument count so it gives false positives which is in my opinion worse then the errors it gave.

Example:

<?php

declare(strict_types=1);

namespace Tests\Unit;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Tests\TestCase;

class User extends Model
{
    public function topics(): HasMany
    {
        return $this->hasMany(Topic::class);
    }
}

class Topic extends Model
{
    public function newEloquentBuilder($query): TopicBuilder
    {
        return new TopicBuilder($query);
    }
}

class TopicBuilder extends Builder
{
    public function category(string $category): self
    {
        return $this->where('category', $category);
    }
}

class LaraStanTest extends TestCase
{
    public function test_larastan(): void
    {
        $user = new User();

        $user->topics()->category(1, false)->get();
    }
}

[canvural]

Ok, thank you for testing again!

I'll look into the custom builder support!