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

MissingTemplateParam for Form classes with data_class #294

Open
iquito opened this issue Dec 19, 2022 · 6 comments
Open

MissingTemplateParam for Form classes with data_class #294

iquito opened this issue Dec 19, 2022 · 6 comments

Comments

@iquito
Copy link

iquito commented Dec 19, 2022

With v5.0 I get errors for all my forms about missing template parameters (those errors did not show up in earlier versions):

MissingTemplateParam - src/UserPasswordSetForm.php:13:7 - UserPasswordSetForm has missing template params when extending Symfony\Component\Form\AbstractType

The error goes away when I add the annotation @extends AbstractType<UserPasswordSetAction> for the class, but as all my classes have a fixed data_class set already this seems like unnecessary boilerplate code. My suggestion would be that the plugin checks if a data_class is set in this way (as recommended by Symfony):

public function configureOptions(OptionsResolver $resolver): void
{
    $resolver->setDefaults([
        'data_class' => UserPasswordSetAction::class,
    ]);
}

This describes the missing template parameter and no additional annotations are needed in that case. As this is defined in code it is also more secure compared to an annotation, which can be wrong.

@zmitic
Copy link
Contributor

zmitic commented Dec 19, 2022

With v5.0 I get errors for all my forms about missing template parameters (those errors did not show up in earlier versions):

True, Psalm4 would make a fallback to mixed but version 5 is stricter.

When it comes to data_class: I made form stubs but to be honest, I have no idea how to make a plugin that would read this value and resolve it as template. Sorry, that is above my skills.

But even if I knew, I don't think it would be valid approach anyway. Forms can be bound to array or even simple string (like TextType). Generics play vital role here for $form->getData(): null|T, events and buildView/finishView methods.

If you are not using either, a quick&dirty fix is this:

/**
 * @extends AbstractType<void>
 */
class MyType extends AbstractType

This is what I did for my own forms; about 100 of them were updated in about 15 minutes, where about 20-30 were using void type. But none has data_class.


I knew that stubs would bring this problem eventually but it is still worth one extra line. For example: if constructor of bound class has non-nullable dependencies, you must use empty_data closure. For example:

public function configureOptions(OptionsResolver $resolver): void
{
   $resolver->setDefaults([
       'empty_data' => function(FormInterface $form) {
   			FormAssert::string($title = $form->get('title')->getData());  // webmozart/assert package

   			return new Blog($title);
   		}
   ]);
}

to play nicely with forms:

class FormAssert extends Assert
{
    protected static function reportInvalidArgument($message): never
    {
        throw new TransformationFailedException(invalidMessage: $message);
    }
}

Symfony will completely ignore data_class here and there are no guarantees about the type, just like there isn't for generics. Use like this would make psalm perfectly happy.

I hope I explained the reasons for generics. It is minor inconvenience but totally worth it for complex forms.

@michnovka
Copy link
Contributor

I still dont understand how to use the template types in various form stubs. So if I have a Form, which consists of various form elements added with $builder->add should I still use @extends AbstractType<void> ? or @extends AbstractType<array> ?

I also have some Form extensions luike this one:

class SearchableEntityExtension extends AbstractTypeExtension
{
    /**
     * {@inheritdoc}
     */
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        if ($builder->getForm()->isRoot()) {
            $builder->addEventSubscriber(new SearchableEntityEventSubscriber());
        }
    }

    /**
     * {@inheritdoc}
     */
    public static function getExtendedTypes(): iterable
    {
        return [FormType::class];
    }
}

I have no idea what template type I should use here, as it seems totally nonsense in this case.

@zmitic
Copy link
Contributor

zmitic commented Jan 8, 2023

I still dont understand how to use the template types in various form stubs.

Use the one of underlying data, can be either object (like User entity) or array like array{first_name: ?string}. If you don't need events, or you don't do $data = $form->getData(); yes, you can use void.

Form extensions can also add their own events and fields, that is why they are typed. void is also fine here if you don't need them. Psalm doesn't support default templates so it is ATM impossible to match default behavior (array) for either of them, hence the requirement.

It might seem like an annoyance but I assure you, it is not. It is important to understand how empty_data works which is a key thing for objects with non-nullable values.

The real problem, something that even Symfony docs are missing (most likely not to confuse users): if that closure returns null, or throws TransformationFailedException, then there will be no data to trigger fields validation. That also means Symfony will not be able to read property validation rules and fields would appear valid. This happened to me few times before I figured what is happening and you can replicate it easy: use empty_data and return null.

Like

// controller
$form = $this->createForm(UserType::class);   // no bound data, you are creating one with empty_data
$form->handleRequest($request);
if ($form->isSubmitted() && $form->isValid()) {
	dd($form->getData()); // <---- valid form, but no data
}
// UserType
public function configureOptions(OptionsResolver $resolver): void
{
   $resolver->setDefaults([
       'empty_data' => fn() => null,
   ]);
}

@michnovka
Copy link
Contributor

Thanks @zmitic , I do get it for the AbstractType then, but what about the form extensions? So say my form extension adds a type like array{first_name: ?string}. If I typehint it properly and use this extension on another type, will psalm benefit from this and "know" that the first_name is actually added via this extension? Otherwise it seems like extensions can always be void, as it has no meaning to use anything specific. Thanks!

@zmitic
Copy link
Contributor

zmitic commented Jan 8, 2023

Extensions can add their own events; just like AbstractType.

I am aware it is rare, but when you need it, it is nice to have it. The following example comes from my actual code, but I cut a few bits for readability:

/** @extends AbstractTypeExtension<AbstractContractPayment>  */
class SharedFieldsExtension extends AbstractTypeExtension
{
    public static function getExtendedTypes(): iterable
    {
        yield PaymentType::class;
        yield RefundType::class;
    }

    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
		// some extra fields here

        $builder->addEventListener(FormEvents::PRE_SET_DATA, function (PreSetDataEvent $event) {
			$data = $event->getData(); // <-  psalm knows it is AbstractContractPayment|null
        	// do something with $data
		});
    }
}

There is also PRE_SUBMIT event here but not shown since it always works with scalars and where that FormAssert jumps in. But these 2 events always come with each other.

In this case, I have 2 entities; Payment and Refund, both extending AbstractContractPayment. That abstract class has fields that are common between them, i.e. it is using class table inheritance. I had to use extension since I am extending 2 form types; if it was just 1, I could go with only getParent().

So this might look like over-engineering but it is needed for some really complex forms. I hope that psalm will get default templates eventually so some of these do not interfere with more common cases.

@zmitic
Copy link
Contributor

zmitic commented Jan 8, 2023

One thing that I forgot: I could have used inherit_data inside RefundType and PaymentType like:

$builder->add('shared', SharedFieldsType::class, [
	'inherit_data' => true,
]);

but this approach doesn't allow events which I have a lot. For this reason alone, extension is the only viable approach.

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