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

Error with form event listeners #295

Open
aszenz opened this issue Dec 20, 2022 · 11 comments
Open

Error with form event listeners #295

aszenz opened this issue Dec 20, 2022 · 11 comments

Comments

@aszenz
Copy link
Contributor

aszenz commented Dec 20, 2022

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
Copy link
Contributor

zmitic commented Dec 20, 2022

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

@aszenz
Copy link
Contributor Author

aszenz commented Dec 20, 2022

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
Copy link
Contributor Author

aszenz commented Dec 20, 2022

Note this happens when chaining addEventListener

That is

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

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

@zmitic
Copy link
Contributor

zmitic commented Dec 20, 2022

@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
Copy link
Contributor Author

aszenz commented Dec 20, 2022

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

@zmitic
Copy link
Contributor

zmitic commented Dec 20, 2022

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
Copy link
Contributor

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
Copy link
Contributor

zmitic commented Dec 20, 2022

@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
Copy link
Contributor

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 symfony/symfony#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
Copy link
Contributor

zmitic commented Dec 20, 2022

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 symfony/symfony#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
Copy link
Contributor

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.

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

No branches or pull requests

3 participants