phpstan / phpstan-symfony

Symfony extension for PHPStan
MIT License
698 stars 89 forks source link

Take into account the dynamic Kernel class #374

Closed Neirda24 closed 8 months ago

Neirda24 commented 8 months ago

The kernel class in tests can be dynamic. But the property Kernel is type hinted with the interface which is not wrong of course but can be limiting.

There is no way (AFAIK) to tell PHPStan that my kernel class is App\Tests\Kernel and that the method xxx() always exists.

It should infer the type from the getKernelClass method in each test to determine the actual type.

At least make it Generic so we can @extends KernelTestCase<ClassOfKernel>

ondrejmirtes commented 8 months ago

Please show a piece of code that makes PHPStan report an issue (and which issue).

Neirda24 commented 8 months ago

How ? It involves the symfony classes which uses $_SERVER and $_ENV global variable ? Should I only provide the userland code that raise an issue ?

ondrejmirtes commented 8 months ago

Yes. PHPStan reports an error for your code that you're trying to solve here. I want to see that piece of code.

Neirda24 commented 8 months ago

Ok.

Given I am in a symfony context (6.4 here but doesn't really matter as it also applies to later versions), with the following :

<?php

namespace App\Tests;

final class Kernel extends \App\Kernel
{
    public function doSomething(): void
    {
    }
}
KERNEL_CLASS='App\Tests\Kernel'
// ...

    public function testSomething(): void
    {
        self::ensureKernelShutdown();
        self::bootKernel();

        static::$kernel->doSomething();
    }

// ...

Produces the following error :

 ------ ------------------------------------------------------------------------------------------------------- 
  Line   MyTest.php                                          
 ------ ------------------------------------------------------------------------------------------------------- 
  123    Call to an undefined method Symfony\Component\HttpKernel\KernelInterface::doSomething().  
 ------ ------------------------------------------------------------------------------------------------------- 
ondrejmirtes commented 8 months ago

There's no way for a PHPStan extension to provide means to override the property type.

Your best bet is a stub file in your project: https://phpstan.org/user-guide/stub-files

That looks something like this:

<?php

namespace Symfony\Bundle\FrameworkBundle\Test;

abstract class KernelTestCase
{

    /**
     * @var \App\Tests\Kernel
     */
    protected static $kernel;

}
Neirda24 commented 8 months ago

Will do on my project but waht could we do as a more global solution ?

Maybe we should wrap the static::$kernel in a getKernel() method ?

Or at least make the KernelTestCase class generic so we can provide the Kernel class we use in our test ?

Neirda24 commented 8 months ago

I also just tried

/**
 * @method static class-string<\App\Tests\Kernel> getKernelClass()
 */
class MyTest extends KernelTestCase
{
}

to specify the Kernel class of the method but it didn't worked out either :/

ondrejmirtes commented 8 months ago

Yes, both are possible. Creating and overriding getKernel() method would be nice, provided projects typically have their own project-specific subclasses. That way you could override getKernel() method with a more specific return type, even native one.

Or at least make the KernelTestCase class generic

Yes, that's also possible, but I don't know how many projects typically have their own Kernel subclass with more methods to warrant making KernelTestCase generic because of the Kernel class name.

ondrejmirtes commented 8 months ago

Just tried the @method approach - it works https://phpstan.org/r/eae3bf1c-1a62-4878-ab00-ae432f9b5b81.

As there's currently no actionable to-do here, I'm closing this issue.

Neirda24 commented 8 months ago

Sorry I just tried like I said and it didn't worked at all. I still get the Call to an undefined method Symfony\Component\HttpKernel\KernelInterface:: doSomething().

Also ok for closing but where should we discuss about the generic approach ? Because Kernel----TestCase it would make sense to have it generic on Kernel no ?

ondrejmirtes commented 8 months ago

Sorry I just tried like I said and it didn't worked at all

Issues like these should be reproducible on phpstan.org/try. So I'd like to see your problem reproduced there.

where should we discuss about the generic approach

I don't have any data about how widespread this problem is. Probably not a lot since you're the first one asking about this :) Making a class generic should be justifiable because it puts a certain burden on all its users.

Neirda24 commented 8 months ago

Ok I think I managed to reproduce teh bug : https://phpstan.org/r/f6bc4123-0911-4961-97b0-f11a55c90c7f

ondrejmirtes commented 8 months ago

I don't see the bug. You're overriding getKernelClass method but you're accessing kernel property.

Neirda24 commented 8 months ago

I understand. But at that point the type should be known to be of type Kernel instead of KernelInterface am I wrong ? Because by overriding that class the new is using that class-string thus the type should be Kernel or children.

ondrejmirtes commented 8 months ago

But all of that code is in KernelTestCase that doesn't have the @method annotation.

See: https://phpstan.org/user-guide/troubleshooting-types

PHPStan doesn’t descend into a called function’s body to see what’s going on there. The native types, the PHPDocs, and the registered dynamic return type and type-specifying extensions are everything that’s used to figure out how the types are going to impacted.

github-actions[bot] commented 7 months ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.