phpstan-strict-rules icon indicating copy to clipboard operation
phpstan-strict-rules copied to clipboard

DynamicCallOnStaticMethodsRule warns when both dynamic and static methods exist

Open szepeviktor opened this issue 2 years ago • 5 comments

In Laravel we have Request->validate() and Request::validate() too.

DynamicCallOnStaticMethodsRule emit an error when it sees Request->validate().

Could you add one more if-s?

szepeviktor avatar Nov 01 '21 14:11 szepeviktor

Could you add one more if-s?

What if? There's no Laravel-specific if in the codebase. Can you please describe the issue in more detail? Why is it reported and why is it a false positive?

ondrejmirtes avatar Nov 03 '21 15:11 ondrejmirtes

Can you please describe the issue in more detail?

AFAIK the dynamic method is simply defined in the source code, the static method it added using Laravel macros which must be using Closure::bind but I do not know what I'm speaking.

What I know is both dynamic Request->validate() and static Request::validate() calls are valid.

I'd like phpstan-strict-rules not to alert me that using the dynamic call should be static.

szepeviktor avatar Nov 03 '21 16:11 szepeviktor

I'd like to ask @canvural to shine a little bit more technical light on this :) Thank you.

ondrejmirtes avatar Nov 03 '21 16:11 ondrejmirtes

@ondrejmirtes This is related to Laravel macros. This is the trait used in Laravel. As you can see it implements both __callStatic and __call So for any macro it's fine to call it with Foo::bar() or (new Foo)->bar()

In Larastan we have an extension for this macros, and it returns a MethodReflection implementation with isStatic set to true. We do this because in PHP it's fine to call static methods from instance and also PHPStan will not report Static call to instance method But by doing this strict rules reports Dynamic call to static method

For me the problem is because MethodsClassReflectionExtension is used for both static and instance calls. So we really can't decide when isStatic should be true or false.

canvural avatar Nov 03 '21 18:11 canvural

I am not sure if I grasp the problem completely as I am not that deep into the whole PHPStan inner workings.

But just for clarification It is currently IMPOSSIBLE to determine (in Larastan?) what is a macro call? and what is not, correct? At least that is what I extract from https://github.com/phpstan/phpstan-strict-rules/issues/140#issuecomment-959827777

For me the problem is because MethodsClassReflectionExtension is used for both static and instance calls. So we really can't decide when isStatic should be true or false.

What would you need to determine the difference? A change in PHPStan or in the strict rules extension? Or is it something that needs to be done in Larastan?

My understanding of the situation so far:

As I experience this problem with the \Illuminate\Database\Eloquent\Builder I will focus on that example for now. The class has the @mixin \Illuminate\Database\Query\Builder annotation. This makes all methods from Query\Builder available in Eloquent\Builder as instance methods.

The static-Calls to where() and other in the manner if Model::where() are handled in the \Illuminate\Database\Eloquent\Model::__callStatic(). From thereon everything is instance access, isn't?

Doesn't the error message Dynamic call to static method .... wrong? Idk where exactly PHPStan gets told that those methods are all static, but I figure that is larastan. I figure that might be the problem. This issue has a lot of insights from the Laravel side: https://github.com/larastan/larastan/issues/1272

Again, I am not sure if I grasp the all the circumstances here. I am new to the whole Laravel world and their incredible complicated and inconsistent way of handling things xD (no hate,.... okay, little hate xD). I am still figuring out what the best way to handle everyday situations like these is. So, please, educate me.

func0der avatar Mar 28 '24 12:03 func0der