psalm / psalm-plugin-symfony

Psalm Plugin for Symfony
MIT License
228 stars 53 forks source link

Error with form event listeners #295

Open aszenz opened 1 year ago

aszenz commented 1 year ago

I'm getting these errors with some form event listeners, not sure what $event is being inferred here.

Parameter $event has wrong type 'Symfony\Component\Form\Event\PostSetDataEvent<mixed>|Symfony\Component\Form\Event\PostSubmitEvent<mixed>|Symfony\Component\Form\Event\PreSetDataEvent<mixed>|Symfony\Component\Form\Event\PreSubmitEvent|Symfony\Component\Form\Event\SubmitEvent<mixed>', 

should be 'Symfony\Component\Form\Event\PostSubmitEvent' (see https://psalm.dev/141)

        ->addEventListener(FormEvents::POST_SUBMIT, function (PostSubmitEvent $event) {
zmitic commented 1 year ago

Can you put your code, i.e. the part of $builder->addEventListener(....)?

aszenz commented 1 year ago

With this code:

->addEventListener(FormEvents::POST_SUBMIT, function (PostSubmitEvent $event) {
    foreach ($this->deliveryConditionsFailedToSave as $deliveryCondition) {
        if ($event->getData() === $deliveryCondition) {
            $event->getForm()->get('code')->addError(
                new FormError("Could not change code from '{$deliveryCondition->getCode()}' to '{$event->getForm()->get('code')->getData()}'. Code '{$deliveryCondition->getCode()}' is in use.")
            );
        }
    }
})

I get this error (on symfony 6.2, php8.1)

ERROR: MismatchingDocblockParamType - src/TradeBundle/Form/Type/DeliveryConditionType.php:40:67 - 
Parameter $event has wrong type 'Symfony\Component\Form\Event\PostSetDataEvent<mixed>|Symfony\Component\Form\Event\PostSubmitEvent<mixed>|Symfony\Component\Form\Event\PreSetDataEvent<mixed>|Symfony\Component\Form\Event\PreSubmitEvent|Symfony\Component\Form\Event\SubmitEvent<mixed>', 
should be 'Symfony\Component\Form\Event\PostSubmitEvent' (see https://psalm.dev/141)

->addEventListener(FormEvents::POST_SUBMIT, function (PostSubmitEvent $event) {
aszenz commented 1 year ago

Note this happens when chaining addEventListener

That is

$builder->add()->addEventListener()

Without chaining it seems to work fine like: $builder->addEventListener()

zmitic commented 1 year ago

@aszenz Yes, I see it now. I forgot to @return FormConfigBuilderInterface<T>.

I can make a fix in 2-3 days but can you brute-edit this file and add

* @return FormConfigBuilderInterface<T>

and see if chaining works until I fix this?

aszenz commented 1 year ago

It seems to cause other errors like `UndefinedInterfaceMethod - SimpleContractlineType.php:63:15 - Method Symfony\Component\Form\FormConfigBuilderInterface::add does not exist

zmitic commented 1 year ago

OK, my bad; I just noticed you were using

$builder->add()->addEventListener()

and I miss-read it as

$builder->addEventListener()->addEventListener()

To help me the fix, can you put entire full calls you make, and all errors? Should be easy fix, just need a way to replicate it.

Updated: on second thought, $builder->add(MyType::class)->addEventListener() will still be working with mixed (default from Symfony), which Psalm5 doesn't really like. It can be templated, not that much of a deal but discussion needed.

@seferov @VincentLanglet What do you guys think? Something like nested generics in AbstractController?

For example:

interface FormBuilderInterface extends \Traversable, \Countable, FormConfigBuilderInterface
{
    /**
     * @template TChildType
     * @template TFormType of FormTypeInterface<TChildType>
     *
     * @psalm-param class-string<TFormType> $type
     *
     * @psalm-return FormBuilderInterface<TChildType>
     */
    public function add(string|FormBuilderInterface $child, string $type = null, array $options = []): static;

I think this one would work but the problem is that $type is nullable. In that case, Symfony makes a fallback to TextType, unless form_type.guesser finds better.

VincentLanglet commented 1 year ago

Didn't really follow this subject but I would say to be careful when introducing generics on a plugin: Other tools (for instance PHPStan) will not support this generic until someone add the same in phpstan-symfony (but the risk is to have someone providing a different template...) That's one of the reason why I stopped using Psalm in favor of phpstan. Because they were incompatible

Symfony started to merge PR with generics, so it could be interesting to start doing PR on symfony code base instead.

zmitic commented 1 year ago

@VincentLanglet

Other tools (for instance PHPStan) will not support this generic until someone add the same in phpstan-symfony (but the risk is to have someone providing a different template...)

The issue is that PHPStan doesn't yet support nested generics (I think this is the correct term) like one in AbstractController. For example:

$data = $this->createForm(CustomerType::class)->getData(); // mixed

but psalm can correctly resolve to null|Customer.

Didn't really follow this subject but I would say to be careful when introducing generics on a plugin:

Sorry, I didn't understand this one. Are you in favor of resolving it via plugin event, or via template annotation (as it is now) or nothing at all (mixed always)?

We could also make an option to disable form stubs if users find this incompatibility with PHPStan a big problem. Or even remove from this repository and put into another.

WDYT; is it worth it? You are more in Symfony circles than I am.

VincentLanglet commented 1 year ago

The issue is that PHPStan doesn't yet support nested generics (I think this is the correct term) like one in AbstractController. For example:

$data = $this->createForm(CustomerType::class)->getData(); // mixed

You mean that if CustomerType is a FormType<Foo>, then $this->createForm(CustomerType::class) should return FormType<Foo> and then getData will return Foo ?

I'm not sure bout the support of PHPStan, I would say it's possible to do this...

Sorry, I didn't understand this one. Are you in favor of resolving it via plugin event, or via template annotation (as it is now) or nothing at all (mixed always)?

I'm fine with template, but it can now be done directly on Symfony first like https://github.com/symfony/symfony/pull/47412

We could also make an option to disable form stubs if users find this incompatibility with PHPStan a big problem. Or even remove from this repository and put into another.

There is an issue with FormView template for instance, because it's not generic in PHPStan but it is in Psalm. Fact is that Psalm supports the syntax

array{value: T}&array<string, mixed>

but PHPStan doesn't, so a stub in PHPStan codebase wouldn't make sens.

I got a recent issue with Psalm 5 since now this tool report "MissingTemplate" errors which didn't exist in Psalm 4. I didn't take a big look, but it's maybe possible to disable with the IssueHandler, something like

<MissingTemplate>
    <class>
       <FormView error="suppress" />
    </class>
</MissingTemplate>

So I would say there is no need to provide something to disable a stubs.

zmitic commented 1 year ago

You mean that if CustomerType is a FormType, then $this->createForm(CustomerType::class) should return FormType and then getData will return Foo ? I'm not sure bout the support of PHPStan, I would say it's possible to do this...

Yes, that one. I tried PHPStan to resolve this, but it always returned mixed. But I am sure eventually it will get fixed.

I'm fine with template, but it can now be done directly on Symfony first like https://github.com/symfony/symfony/pull/47412

There has been few attempts of merging form stubs into Symfony itself, but stof gave some good arguments.

From my POV, after 2-3 years of using these stubs, I have never had a problem. Doesn't mean they don't exist but I just didn't encounter them even with complex compound forms and transformers.

Did you have problems with them, assuming event listeners were used? If so, can you paste an example?

<MissingTemplate>

Honestly, I didn't even know about this one, thanks. Today I tried V5 and from zero errors, got 212; only 5 coming from me forgetting template on AbstractTypeExtension.

But with this, compatibility with V4 can be kept and not deter new users.

VincentLanglet commented 1 year ago

Did you have problems with them, assuming event listeners were used? If so, can you paste an example?

I essentially had problem because stub was added to PHPStan or Psalm, and I had to add it to the other. Currently the only issue I have is the fact the Form-generics doesn't exists in PHPStan. But never used the eventListener-related ones.