-
Notifications
You must be signed in to change notification settings - Fork 95
feat: Support specifying type of data_class in forms #337
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
Conversation
7d257b4 to
a18b756
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've given a compelling list of arguments! Looking forward to merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 88da905
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what these tests are supposed to do, they're not referenced from anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used from here: https://github.com/siketyan/phpstan-symfony/blob/a18b756621163b0040a3c3f5a58a2f24b3652f48/tests/Stubs/Symfony/Component/Form/FormFactoryAwareClass.php#L20.
Although it is for testing purpose to confirm the FormFactoryInterface accepts DataClass for DataClassType form type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s not how testing works here 😊 Please see https://phpstan.org/developing-extensions/testing to know how to test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this? dcf9b4f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These complex generics should be tested with TypeInferenceTestCase. To make sure they behave like you expect them to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time a new class is made generic, the class name should be added here:
phpstan-symfony/extension.neon
Lines 14 to 18 in 1da7bf4
This list will be made empty with the next major version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in da2cb98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look where is the truth, but the Psalm template returns ?TData
https://github.com/psalm/psalm-plugin-symfony/blob/5.x/src/Stubs/common/Component/Form/FormInterface.stubphp#L12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be null only if we omit a initial value of the form.
https://github.com/siketyan/phpstan-symfony/blob/88da905c8a29f791fa9ee78ec5f83f2980f8e809/stubs/Symfony/Component/Form/FormFactoryInterface.stub#L17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So it means psalm should improve his typing ^^
Thank you!
@siketyan @ondrejmirtes After update to 1.3.0 I get now following errors:
Parameter $form of method App\Form\Type\TypeaheadType::buildView() has invalid type Symfony\Component\Form\TData.
I haven't much experience with the generics. I am only on phpstan level 6 with disabled checkGenericClassInNonGenericObjectType and checkMissingIterableValueType.
The form types have no data class. So I tried to add following phpdocs to the classes, but it doesn't help:
/**
* @extends AbstractType<null>
*/
/**
* @extends AbstractType<mixed>
*/
Is there a bug on your side or what make I wrong?
Please show a piece of code for which PHPStan reports this error:
Parameter $form of method App\Form\Type\TypeaheadType::buildView() has invalid type Symfony\Component\Form\TData.
<?php
namespace App\Form\Type;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\OptionsResolver\OptionsResolver;
/**
* @extends AbstractType<mixed>
*/
class TypeaheadType extends AbstractType
{
public function configureOptions(OptionsResolver $resolver): void
{
$resolver->setDefaults([
'definitionType' => 'class',
'definitionId' => null,
'field' => null,
'multiple' => false,
'type' => '',
'documentId' => null,
]);
}
public function buildView(FormView $view, FormInterface $form, array $options): void
{
if (!$options['definitionId'] || !$options['field']) {
throw new \InvalidArgumentException('Typeahead: No definitionId or field is set');
}
$view->vars = array_replace_recursive($view->vars, [
'definitionType' => $options['definitionType'],
'definitionId' => $options['definitionId'],
'field' => $options['field'],
'multiple' => $options['multiple'],
'documentId' => $options['documentId'],
'attr' => [
'class' => 'js-typeahead ' . $options['type'],
]
]);
}
public function getParent(): string
{
return TextType::class;
}
}
Fixed: 7e78605
There was a weird bug because all the methods are re-defined in AbstractType even if they're in the FormTypeInterface ancestor. So the stub file needs to repeat them too.
Thank you for the quick fix @ondrejmirtes and sorry for my mistake @blankse
@ondrejmirtes Thank you for the quick fix 👍🏼
You can now specify what type of data_class the FormType have as the generic parameter of FormTypeInterface or AbstractType.
Example:
Also, you can validate mismatch between data_class in FormType and the initial data:
Then you can get appropriate type of the data: