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

Psalm errors with Attributes phpstan-type declaration #701

Closed
svianney opened this issue Oct 7, 2024 · 4 comments · Fixed by #704
Closed

Psalm errors with Attributes phpstan-type declaration #701

svianney opened this issue Oct 7, 2024 · 4 comments · Fixed by #704

Comments

@svianney
Copy link

svianney commented Oct 7, 2024

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.

@nikophil
Copy link
Member

nikophil commented Oct 18, 2024

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

@svianney
Copy link
Author

svianney commented Oct 24, 2024

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 💪

@nikophil
Copy link
Member

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

@svianney
Copy link
Author

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>

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

Successfully merging a pull request may close this issue.

2 participants