Forward calls on Model when subclass is not known

[spawnia]

• Added or updated tests
• Documented user facing changes
• Updated CHANGELOG.md

Changes

Avoid false-positive when calling static builder methods such as ::find() on Model classes where the concrete subclass is not yet known.

Breaking changes

No, allows more valid programs.

[canvural]

I think it's failing because the type hint is Model If you give a specific model like User or Account it should work.

What is the use case for this?

[spawnia]

I think it's failing because the type hint is Model If you give a specific model like User or Account it should work.

Right, i saw that in the test case above. It should also work with the generic Model type, though.

What is the use case for this?

I hit this in Lighthouse, where we interact with user-defined models in a generic way. See https://github.com/nuwave/lighthouse/blob/f32243e71930e515a4b8661520c67d4f1ba04416/src/Schema/Directives/DeleteDirective.php#L60
By nature of it being a library, the concrete type can not be known.

[canvural]

This extension is responsible for finding the missing methods on models. But this check prevents it running for Model (Model is not subclass of Model)

So you can try to change that check to if ($classReflection->getName !== Model::class && ! $classReflection->isSubclassOf(Model::class)) and then you can see what is the output and try to improve on that.

[canvural]

Seems like it works with my suggested change. But you have to remove the @mixin here. I added that to simulate some cases where users would run laravel-ide-helper and it adds this mixin. You can see the discussion at #555

So, I guess @mixin \Eloquent needs to be ignored somehow.

[spawnia]

@canvural your proposed fix does indeed, work - thank you!

I found that this does allow some invalid programs:

\Illuminate\Database\Eloquent\Model::find(1);

/** @var class-string<\Illuminate\Database\Eloquent\Model> */
$model = 'Illuminate\Database\Eloquent\Model';
$model::find(1);

I dug around PHPStan's ClassReflection to check if the source of the class can be known. For example, here it can be known for sure that the passed in Model is an instance and calling ::find() works:

    public function testFindOnGenericModel(Model $model): ?Model
    {
        return $model::find(1);
    }

The following example is ambigious:

    /**
     * @param  class-string<Model>  $modelClass
     */
    public function testFindOnModelClassString(string $modelClass): ?Model
    {
        return $modelClass::find(1);
    }

Is there a way to specify a class-string must be a subclass of the given type?

[spawnia]

I guess my branch was not up to date, the mixin does seem to be causing problems.

[spawnia]

@canvural opened an issue with PHPStan that might help to solve this use case. phpstan/phpstan#3256

Pragmatically, i think it should be extremely rare that developers actually try to call something like Model::find(). If they do, the error is very immediate and clear:

PHP Error:  Cannot instantiate abstract class Illuminate/Database/Eloquent/Model in /var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php on line 1634

[canvural]

Hi,

PHPStan 0.12.24 is released. So I updated master branch, and added Eloquent to mixinExcludeClasses config option.

So if you rebase this branch on master the tests should pass. And if you can update the changelog we can merge this 👍

[spawnia]

Nicely done @canvural. It's unfortunate the types generated by laravel-ide-helper are so inaccurate.

[mfn]

It's unfortunate the types generated by laravel-ide-helper

We can improve it, what needs to be done ? 😄

[spawnia]

@mfn just to be clear, i was not bashing laravel-ide-helper. I think it does its part quite well actually - provide useful type hints that play nicely with IDE's.

Many of the nuances of Laravel's magic can't really be expressed correctly through a nominal type system. Other parts can be expressed only through non-standard, PHPStan-specific annotations, for which proper IDE support does not exist.

I was quite happy to see you pick up work on the project, there is a large backlog of issues and open PR's - lot's to do 😉