Method calls on custom query builder to public parent methods are wrongly reported as private

[nagmat84]

• Larastan Version: 2.1.11
• --level used: 7
• Pull request with failing test: <none>

Description

If one creates a trait to equip a model with a custom query builder, then methods calls on the custom query builder which are defined in Illuminate\Database\Query\Builder are wrongly reported as private.

Laravel code where the issue was found

I prepared the following GITHub repository with a minimal Laravel application: https://github.com/nagmat84/larastan-example. Step to reproduce the problem:

1. Checkout the repository
2. Install all dependencies via composer install
3. Run PHPStan via composer phpstan

You will get the following output

 ------ ---------------------------------------------------------------------------------------------------------- 
  Line   Http/Controllers/Controller.php                                                                           
 ------ ---------------------------------------------------------------------------------------------------------- 
  18     Call to private method whereBetween() of parent class Illuminate\Database\Eloquent\Builder<TNodeModel of  
         App\Models\Contracts\Node&Illuminate\Database\Eloquent\Model>.                                            
 ------ ----------------------------------------------------------------------------------------------------------

However the method is not private.

[szepeviktor]

Method App\Models\Node::newEloquentBuilder() should return App\Database\NodeBuilder<App\Models\Node> but
         returns App\Database\NodeBuilder<App\Models\Contracts\Node&Illuminate\Database\Eloquent\Model>

🧪 Hello Matthias! 👋🏻
I think union types are not (yet) supported.

[nagmat84]

First, there is no union type. I assume you mean intersection type.

However, this is not the problem which this issue is about. The problem is the first reported error by Larastan as shown in the issue description.

[szepeviktor]

I assume you mean intersection type.

Yes. I never use the name, only | and &

[szepeviktor]

I think Larastan does not support this much abstraction.
class NodeBuilder extends EloquentBuilder

[szepeviktor]

Try going back to this simplicity, this is supported.
https://github.com/nunomaduro/larastan/blob/master/UPGRADE.md#custom-eloquent-builders

[nagmat84]

This is exactly as in the the example.

The template:

use Illuminate\Database\Eloquent\Builder;

/**
 * @template TModelClass of \Illuminate\Database\Eloquent\Model
 * @extends Builder<TModelClass>
 */
class CustomBuilder extends Builder
{
}

My example:

use App\Models\Contracts\Node;
use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
use Illuminate\Database\Eloquent\Model;

/**
 * @template TNodeModel of Model&Node
 * @phpstan-extends EloquentBuilder<TNodeModel>
 */
class NodeBuilder extends EloquentBuilder
{
}

Except for difference in names it is exactly the same.

My gut feeling tells me that the problem is related to the trait. The template directly defines the methods like newEloquentBuilder directly on the final model class whereas my example uses a trait. I guess this is the problem.

Try going back to this simplicity

This is unfortunately impossible, because there is a reason for NodeTrait. We have several such models which requires it and not using a trait would result in very much code duplication. Please keep in mind that this is only a minimal example. The real NodeTrait is several hundreds LoC. It is an absolute no-go to repeat that on every model.

[szepeviktor]

My gut feeling tells me that the problem is related to the trait.

For a moment please try to move the trait to everywhere it is used.

[nagmat84]

That is not going to happen. Rather we migrate the whole project away from Laravel to Symfony.

Not only due to this problem at hand, but because we had a long history of fuck-ups due to Laravel's stupid architecture. (Yes, I am aware that Laravel and Larastan are two different projects.) In the past I used Symfony for other project which had a much more complex DB model than the current Laravel project and I never ever encountered any of the many problem we already had with Laravel despite the fact that the current project is much simpler. (I don't want to know what would have happened, if we had tried to realize the other projects in Laravel.)

So before I start duplicating code and create a maintainability hell for the future, I rather take the effort to migrate the whole project.

[szepeviktor]

because we had a long history of fuck-ups due to Laravel's stupid architecture

AFAIK Laravel filled the gap between existing childish "frameworks" and Symfony.
90+% of developers are not able to use Symfony.

[nagmat84]

BTW, I wonder why

https://github.com/nunomaduro/larastan/blob/4cbe40866c2c86bbd27f2424ab735ac6ad10338c/src/Methods/BuilderHelper.php#L201

uses getNativeMethod on the reflection of the model class. Again, my gut feeling tells me that it should be getMethod. I haven't spent much time on understanding the difference between getNativeMethod and getMethod, but it seems that traits are part of the difference.

The line dates back to commit af4094a when the whole BuilderHelper was written and has not been changed since then. I wonder wether the decision for getNativeMethod instead of getMethod was made intentionally or accidentally.

If this is indeed the cause for the bug, then it does not solely affect this line but all other lines in BuilderHelper which also use getNativeMethod.

[nagmat84]

I updated my example at https://github.com/nagmat84/larastan-example. It is now even more trivial and does not use any interface or any intersection type. But the errors are still there. IMHO this needs to be fixed, because it is a very typical pattern that models are augmented via traits.

[szepeviktor]

See the results online: https://github.com/nagmat84/larastan-example/actions/runs/2578269170

[canvural]

Hi,

I cannot reproduce Call to private method whereBetween() of parent class ... error on the example repo.

About the other errors;

• Errors like Method App\Models\Node::newCollection() should return App\Database\NodeCollection<App\Models\Node> but returns App\Database\NodeCollection<Illuminate\Database\Eloquent\Model>. in the trait is kinda expected. This is a PHPStan limitation. Currently there is no way to pass generic types in object initialization. For example in return new NodeBuilder($query); You can follow this issue for more information.
• If you rename generic types TNodeModel to TModelClass it gets rid of the other error. I know this could be improved, but for now this is a workaround.
• Last remaining error is because newModelQuery does not have correct stubs in Larastan. But for example if you change it to Node::query() you'll see it works. I'll try to update the stubs later.

[canvural]

BTW, I wonder why

https://github.com/nunomaduro/larastan/blob/4cbe40866c2c86bbd27f2424ab735ac6ad10338c/src/Methods/BuilderHelper.php#L201

uses getNativeMethod on the reflection of the model class. Again, my gut feeling tells me that it should be getMethod. I haven't spent much time on understanding the difference between getNativeMethod and getMethod, but it seems that traits are part of the difference.

The line dates back to commit af4094a when the whole BuilderHelper was written and has not been changed since then. I wonder wether the decision for getNativeMethod instead of getMethod was made intentionally or accidentally.

If this is indeed the cause for the bug, then it does not solely affect this line but all other lines in BuilderHelper which also use getNativeMethod.

It is intentional and has nothing to do with the errors you got. getMethod also calls the registered extensions to find a method reflection. If we'd use it there it'd cause infinite loops. getNativeMethod is perfectly fine there.

[nagmat84]

I cannot reproduce Call to private method whereBetween() of parent class ... error on the example repo.

Sorry, for that. I pushed an even smaller MWE after @szepeviktor suggested in his post that the intersection types might be the problem (even though he wrongly called them union types). In doing the original error changed from "call to private method" into "call to undefined method".

I followed your advices (see below) and also reverted the example repo to the full example (note that the default branch has changed).

* If you rename generic types `TNodeModel` to `TModelClass` it gets rid of the other error. I know this could be improved, but for now this is a workaround.

Great. This did not only solve the problem "call to undefined method" for the branch trivial_example, but also solved the problem "call to private method" for the branch full_example and any other error. Now, PhpStan is even happy with the intersection types.

I don't understand why one must not rename the template parameter, but I can live with that restriction. IMHO, there should be a bold, eye-catching warning in the documentation that one must not rename inherited template parameters.

* Last remaining error is because `newModelQuery` does not have correct stubs in Larastan. But for example if you change it to `Node::query()` you'll see it works. I'll try to update the stubs later.

Yes, this is the only error which also remains for the branch full_example. It would be nice to get a fix for that.

[canvural]

Yes, this is the only error which also remains for the branch full_example. It would be nice to get a fix for that.

Fixed in #1299

[nagmat84]

I can confirm that #1299 fixed it, see here https://github.com/nagmat84/larastan-example/runs/7161581815?check_suite_focus=true.

I will now use what I have learned (i.e. that you must not rename the template parameter!) and apply it to our actual code. May be there are still other problems, but that another issue then.

Is there any chance that #1299 gets back-ported to Larastan 1.x. Our project is still stuck at Laravel 8 due to various reasons.

[nagmat84]

Unfortunately, the problem is back again in #1285 after I have added a test class for custom relations.