-
Notifications
You must be signed in to change notification settings - Fork 93
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.
* @param class-string<TFormType> $type | ||
* @param TData $data | ||
* @param array<string, mixed> $options | ||
* |
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
/** | ||
* @extends AbstractType<DataClass> | ||
*/ | ||
class DataClassType extends AbstractType |
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
* @param TData $data | ||
* @param array<string, mixed> $options | ||
* | ||
* @phpstan-return ($data is null ? FormInterface<null|TData> : FormInterface<TData>) |
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.
namespace Symfony\Component\Form; | ||
|
||
/** | ||
* @template TData |
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
featureToggles: | |
skipCheckGenericClasses: | |
- Symfony\Component\OptionsResolver\Options | |
- Symfony\Component\Security\Core\Authorization\Voter\Voter | |
- Symfony\Component\Security\Core\User\PasswordUpgraderInterface |
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
*/ | ||
interface FormInterface extends \ArrayAccess, \Traversable, \Countable | ||
{ | ||
|
||
/** | ||
* @return TData |
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:
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:
Is there a bug on your side or what make I wrong? |
Please show a piece of code for which PHPStan reports this error:
|
|
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: