foundry icon indicating copy to clipboard operation
foundry copied to clipboard

Psalm errors with Attributes phpstan-type declaration

Open svianney opened this issue 1 year ago • 1 comments

Hi !

I should have opened this issue a month ago (sorry about that) since I commented here thinking it was related.

As mentionned in the linked comment, since 2.0.8 I started getting these kind of Psalm errors:

InvalidArgument - src/DataFixtures/AppFixtures.php:128:70 - Argument 1 of App\Foundry\Factory\Node\EndConversationNodeFactory::createOne expects Zenstruck\Foundry\Persistence\Attributes, but array{position: App\Entity\Position, script: App\Entity\Script} provided (see https://psalm.dev/004)

From what I understand: Psalm doesn't seem to appreciate the fact that I pass an array instead of an instance of a non existing class Attributes to my factories like this:

EndConversationNodeFactory::createOne([
    'script' => $script,
]);

After some investigations it seems that these "errors" have been introduced by this commit

and particularly by these added lines in the PersistentProxyObjectFactory image

if I remove @method annotations, everything gets back to normal on my end, I'm trying to understand what's happening and my best guess is that @method annotations should not use phpstan aliases like Attributes.

Hope I was clear enough. If you need me to provide more information, just ask, I'd be happy to do so.

svianney avatar Oct 07 '24 10:10 svianney

Hello,

would you mind to test this PR please? I've finally decided to create a psalm extension, you may need to add this to your psalm.xml:

    <plugins>
        <pluginClass class="Zenstruck\Foundry\Psalm\FoundryPlugin"/>
    </plugins>

I've removed all those @method from PersistentProxyObjectFactory, so you should not have the problem anymore.

By the way, I think we're abusing PHPStan's pseudo types, and everywhere we're using them, I think we should prefix the annotation with @phpstan-*. I may do this in a further PR

nikophil avatar Oct 18 '24 15:10 nikophil

I'm sorry I missed the notification of your comment and I have not been of any help. I've just seen the package upgrade to 2.2.0 on my machine after a composer update and no psalm errors appeared. (I had not used your Plugin yet) I use psalm a the most strict level.

Then I tried with and without the plugin and both case gave me no error, I'm not sure what the plugin does and I won't use it till I'm sure I need it.

Thanks for the fix. Thanks for your good work 💪

svianney avatar Oct 24 '24 15:10 svianney

thanks for your answer! do you have that bunch of ugly annotations above your factories?

image

without them and without the plugin, I cannot manage to get a simple psalm analysis right

nikophil avatar Oct 24 '24 17:10 nikophil

No I don't, my factories are as simple as that image I use these plugins though, but I'm pretty sure they are not particularly helping in this case.

    <plugins>
        <pluginClass class="Psalm\SymfonyPsalmPlugin\Plugin">
            <containerXml>var/cache/dev/App_KernelDevDebugContainer.xml</containerXml>
            <containerXml>var/cache/dev/App_KernelTestDebugContainer.xml</containerXml>
            <twigCachePath>var/cache/dev/twig</twigCachePath>
        </pluginClass>

        <pluginClass class="Weirdan\DoctrinePsalmPlugin\Plugin"/>
        <pluginClass class="Psalm\PhpUnitPlugin\Plugin"/>
    </plugins>

svianney avatar Oct 29 '24 13:10 svianney