Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DynamicCallOnStaticMethodsRule warns when both dynamic and static methods exist #140

Open
szepeviktor opened this issue Nov 1, 2021 · 5 comments

Comments

@szepeviktor
Copy link

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 szepeviktor changed the title DynamicCallOnStaticMethodsRule warns when both dynamic and static call exists DynamicCallOnStaticMethodsRule warns when both dynamic and static methods exist Nov 1, 2021
@ondrejmirtes
Copy link
Member

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?

@szepeviktor
Copy link
Author

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.

@ondrejmirtes
Copy link
Member

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

@canvural
Copy link

canvural commented Nov 3, 2021

@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.

philbates35 added a commit to philbates35/laravel-starter that referenced this issue Jan 27, 2024
The following route:

    Route::get('/', function(\Illuminate\Contracts\Filesystem\Filesystem $factory) {
        return $factory->download('foo.csv');
    });

currently results in the following false-positive error:

     ------ ------------------------------------------------------------------------------------
      Line   routes/web.php
     ------ ------------------------------------------------------------------------------------
      22     Dynamic call to static method Illuminate\Filesystem\FilesystemAdapter::download().
     ------ ------------------------------------------------------------------------------------

See phpstan/phpstan-strict-rules#140
philbates35 added a commit to philbates35/laravel-starter that referenced this issue Jan 31, 2024
The following route:

    Route::get('/', function(\Illuminate\Contracts\Filesystem\Filesystem $factory) {
        return $factory->download('foo.csv');
    });

currently results in the following false-positive error:

     ------ ------------------------------------------------------------------------------------
      Line   routes/web.php
     ------ ------------------------------------------------------------------------------------
      22     Dynamic call to static method Illuminate\Filesystem\FilesystemAdapter::download().
     ------ ------------------------------------------------------------------------------------

See phpstan/phpstan-strict-rules#140
See larastan/larastan#1272
@func0der
Copy link

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 #140 (comment)

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: larastan/larastan#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants