Using this to call a static member method not always being detected

UweKeim

Consider this code:

<?php declare(strict_types = 1);

function TestCallable(callable $c) : void
{
}

abstract class AB
{
    public static function Foo() : void
    {
        self::Bar();
        
        TestCallable(function ()
        {
            $this->Bar(); 
        });
    }

    public static function Bar() : void
    {
    }
}

Looking at the call to $this->Bar();, PHP Tools does not detect the misuse of $this-> (which should be self::). PHPStan does.

My suggestion

Please let PHP Tools also detect this issue and warn/notify me about it.

JakubMisek

UweKeim I'm looking into this over and over .. the problem is, it's completely valid PHP code with no notices or errors, and sometimes it might be even intended.

$this-> is not the same as self::, it has a different semantic.

$this->foo() would call an overridden foo() in a subclass - so it takes class hierarchy into account.
self::foo() would call the foo() on the current class.

I've seen a lot of code taking advantage of $this.

UweKeim

A similar issue that is not being detected:

<?php declare(strict_types = 1);

class Test3
{
    public function Foo() : void
    {
    }

    public static function Bar() : void
    {
        self::Foo();
    }
}

Here, the call to Foo() cannot be done with self::Foo();, because a static method cannot call an instance method.

PHP Tools does not detect this wrong call, PHPStan does:

Static call to instance method Test3::Foo().

My suggestion

I also would love such cases to be detected and notified.

JakubMisek

UweKeim back to the original issue, we are reporting the misuse of self::

JakubMisek

Thank you for the suggestion! adding it to our to-to.

UweKeim

How about building a general interface to connect to external tools like PHPStan?

PhpStorm has such a functionality and it works great.

Don't know whether this is way too much out of scope for your extension, but I would love to have it anyway 🙂.

I could even imagine that if you implement such a functionality, you could make it much easier to configure than it currently has to be configured inside PhpStorm (in fact, a zero-config functionality would just rock).

JakubMisek

UweKeim it makes sense, basically disabling our diagnostics and parsing the output of phpstan;

~~We can do that, I thought there is already phpstan extension tho?~~

UweKeim

JakubMisek Thanks, Jakub. Actually I'm not aware of such an extension for Visual Studio.

A quick Google search also reveals one extension for Visual Studio Code only.

Maybe I'm missing something here?!?

JakubMisek

UweKeim you're right, there is no phpstan extension for VS yet, and the one for VSCode would need some improvements.

UweKeim

Just a small follow-up:

Recently I'm developing most of my PHP projects both in PhpStorm and PHP Tools in parallel.

(Debugging in PHP Tools is way more easy and way better than in PhpStorm).

Today I had the situation that PHP Tools did detect two errors that PhpStrom did not detect.

So in this context, regarding this quote above of Jakub:

...basically disabling our diagnostics...

I'm strongly recommending that you do not disable your own diagnostics and solely use PHPStan output.

Instead you should keep (and constantly improve) your own diagnostics and add PHPStan output additionally on top.

Otherwise my today's errors would go totally undetected.

JakubMisek

UweKeim you are right, we're working with all our users to fine-tune the diagnostics. And I agree we can provide very useful warnings and code fixes.

UweKeim

Thank you, Jakub.

I'm not that expert in PHP so if you say this something not to fix, I'm (of course!) totally OK with it.

In C# ReSharper that are options to add comments to instruct ReSharper to not output a certain warning and omit it. Maybe something like that would be helpful for PHP Tools in cases like this one, too?

JakubMisek

UweKeim we have the configuration for diagnostics as well (https://docs.devsense.com/vs/code%20validation/configuration) :)

Although, calling static methods using $this-> is more like a feature. Sure it can be unintended sometimes, but you won't get any warning or error during runtime.

I'm wondering if people are not complaining about this diagnostic with PHPStan /

JakubMisek

I'll mark it as "solved", and maybe come up with a Code Action to change $this-> to self::, but probably not as a warning. We've tried that and there were hundreds of falsy warnings in frameworks like Symfony etc.