Discussion: better (?) typing suggestion for AbstractController::createForm

[zerkms]

Current definition is the following:

psalm-plugin-symfony/src/Stubs/5/Bundle/FrameworkBundle/Controller/AbstractController.stubphp

Lines 18 to 26 in fb801a9

	/**
	* @template TData
	* @template TFormType of FormTypeInterface<TData>
	*
	* @psalm-param class-string<TFormType> $type
	*
	* @psalm-return FormInterface<TData>
	*/
	protected function createForm(string $type, $data = null, array $options = []): FormInterface {}

I suggest to change it to:

    /**
     * @template TData
     * @template TFormType of FormTypeInterface<TData>
     *
     * @psalm-param class-string<TFormType> $type
     * @psalm-param TData $data
     *
     * @psalm-return FormInterface<null|TData>
     */
    protected function createForm(string $type, $data = null, array $options = []): FormInterface {}

What has changed:

1. TData $data parameter added
2. null|TData generic parameter changed for the return type

Why: this would allow check the $this->createForm(FormType::class, ['rubbish']) compatibility: if the FormType does not extend AbstractType<array{0: 'rubbish'}> then it will be marked as incompatible.

Thoughts?

[zmitic]

Good idea, it makes perfect sense. But why return nullable FormInterface<null|TData>?

FormInterface:getData already returns null|TData. It has to, because create methods don't pass $data parameter, which then triggers empty_data callback.

[zerkms]

But why return nullable FormInterface<null|TData>

Because createForm's second argument $data is declared as having = null by default in symfony, and having generic parameter nullable was the only way I could find working to make psalm happy about type-checking it.

FormInterface:getData already returns null|TData. It has to, because create methods don't pass $data parameter, which then triggers empty_data callback.

Indeed, but psalm wasn't accepting it 🤷 So it was more like - overcoming the psalm limitation (or a bug?), than a thoughtful decision :-D