Property check

[canvural]

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

This PR adds support for checking arguments passed to methods that expect model properties. For example

// `foo` does not exist in `users` table. So an error will occur
User::where('foo', 'bar')->get();

How it works?

This PR uses the types introduced in #657 by @Daanra But unlike that PR there is no check done in type level. Instead there are 2 new rules (currently, more to come) that check severy method call and inspects if any of the arguments have the new model-property or generic model-property<Model> type. If so the parameter will be checked against the model property.

Using in custom methods

It's not limited to Laravel core methods. Any custom method can typehint that it expects properties of a model with model-property<Model> typehint.

class HelloWorld
{
	/**
	 * @phpstan-param model-property<\App\User> $property
	 */
    public function hello(string $property)
    {
		// here we can be sure `$property` is actually a property of User model
    }
}

// Will produce error. `foo` is not a \App\User property
(new HelloWorld)->hello('foo');

Stubs

New types added to Laravel core methods by the help of stubs. Currently not all methods have the new type in docblocks. But it's fairly easy to add them to stubs so I'm thinking preparing a guide of how-to, and ask people to contribute. Would be a cool and easy way to contribute to OSS and Hacktoberfest.

Next steps

• We need to test it on more projects. I already did some testing on some open-source Laravel projects + my work projects and fixed couple of bugs. But I'm sure there are more edge cases 😄
• I'll add property checking to dynamic wheres.

// `whereFoo` will look at the `users` column for `foo` column. We can report if this does not exists
User::whereFoo('bar');

• New rule for variable/property assigning. If a variable has a type for model-property<Model> we should check the assigned value.

/** @var model-property<\App\User>` */
$property = 'foo';

// Should error if `foo` is not a property of `\App\User`

// Same here as `someProperty`
$someObject->someProperty = 'foo';

[canvural]

@Daanra Can you do a review?

[Daanra]

@canvural Yes. I'll try to make some time either tomorrow or the day after.

[Daanra]

I'm getting quite a few false positives when testing this on some of my projects.

The main problems comes from columns that are present on pivot tables. E.g.:

$user->groups()->where('group_joined_at', '>=', now()->subWeek())->get();

I think, for now, the easiest would be to just return no errors in the rule when the method is called on an instance of BelongsToMany. And perhaps in a later iteration we could improve this.

[canvural]

I'm getting quite a few false positives when testing this on some of my projects.

The main problems comes from columns that are present on pivot tables. E.g.:

$user->groups()->where('group_joined_at', '>=', now()->subWeek())->get();

I think, for now, the easiest would be to just return no errors in the rule when the method is called on an instance of BelongsToMany. And perhaps in a later iteration we could improve this.

Isn't that code trying to find the group_joined_at at the Group model? Don't you need wherePivot to search the pivot table?

[Daanra]

Isn't that code trying to find the group_joined_at at the Group model? Don't you need wherePivot to search the pivot table?

Under the hood a join is performed and the where clause will match any joined table. wherePivot seems to just call where under the hood and automatically prefixes the column name with the name of the pivot table to prevent ambiguity. So I suppose in all scenario's it's better to use wherePivot if you're using a pivot column, but where is also valid if the column name is unique with respect to the joined tables.

I'm fine with throwing an error when using ->where($pivotColumn), but maybe we can at least improve the error message to suggest that if the column exists on a pivot table that ->wherePivot should be used instead?

[canvural]

I'm fine with throwing an error when using ->where($pivotColumn), but maybe we can at least improve the error message to suggest that if the column exists on a pivot table that ->wherePivot should be used instead?

That'd be a good idea! I'll try to implement that.

[canvural]

@Daanra How should we display the error for that case? There may be two reasons why column does not exists. First it can be that column really doesn't exist in related model. Second case is when it doesn't exist in pivot table. But we really don't know that.

So should we display 2 errors for that line? One for missing property. Second, a warning about that column name is ambiguous and tell user to prefix it with table name or use wherePivot.

Or just warn about the ambiguity of the colum name?

[Daanra]

So should we display 2 errors for that line? One for missing property. Second, a warning about that column name is ambiguous and tell user to prefix it with table name or use wherePivot.

I'm personally not a fan of displaying two errors. I think in this scenario a single error message with a small hint appended is best. Something along the lines of:

Property 'foo' does not exist in App\\User model. If 'foo' exists as a column on the pivot table, then use 'wherePivot' instead.

This hint should only be appended when the query is called on a subclass of BelongsToMany

[Daanra]

LGTM!

I suppose we still need some docs before releasing this though.

[canvural]

Yeah, we need docs. I'm working on it 👍

Plus we need to make sure every Laravel core function has the new type model-property in the stubs. Thinking about making a new issue for that and get some help from Hacktoberfest power 😄

And there is still stuff to do I wrote in the Next Steps section at the PR description.

[mfn]

I just read https://github.com/nunomaduro/larastan/releases/tag/v0.6.5 and upgraded:

$ ./composer.phar show nunomaduro/larastan|grep versions
versions : * v0.6.5

This example does not report anything my codebase using 0.6.5:

        Profile::where('column_does_not_exist', 'true')->get();

What are the requirements to make this work? How are the properties inferred?

Thanks!

[Daanra]

@mfn Did you add:

parameters:
    checkModelProperties: true

to your config?

[mfn]

No 😢

It was not mentioned on https://github.com/nunomaduro/larastan/releases/tag/v0.6.5 (only that it's "beta") and when I read https://github.com/nunomaduro/larastan/blob/master/docs/rules.md#modelpropertyrule I did not scroll to the last part because, again, it just says "beta" at the beginning but not mentions that it's actually disabled (and I saw the code example and was eager to see this in action).

TL;DR: works great, thank you 🙇

[mfn]

It seems to ignore columns entirely which use the explicit table prefix, i.e. they contain a dot: ->where('tablename.column_does_not_exist', …

Is this expected / a limitation or should I open an issue?

[canvural]

Hi @mfn ,

I'll try to update the docs so it'll mention earlier that it is disabled by default 👍

Is this expected / a limitation or should I open an issue?

Yes, this is expected. If the value contains a dot we simply ignore the check. Because there is no good way to determine which model it should be checked on.

This can be improved maybe. By comparing the model's table and the table name given in the argument. But it'll not work in every case.

[mfn]

Damn 😄

In our projects it's a requirement to us the "full form", because it happens to easily that joins shadow same-named columns and the only way to avoid this was the brute-force approach to make a rule to always use the table name too.