-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Add AsFormType
attribute to create FormType directly on model classes
#60563
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
base: 7.4
Are you sure you want to change the base?
Conversation
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
82ed4dc
to
2cfe438
Compare
$commandDefinition = new Definition(DebugCommand::class, [ | ||
new Reference('form.registry'), | ||
[], | ||
[], | ||
[], | ||
[], | ||
null, | ||
[], | ||
]); |
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.
Why this change ?
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.
If I dont, I got this error on tests:
1) Symfony\Component\Form\Tests\DependencyInjection\FormPassTest::testAddTaggedTypesToDebugCommand
Symfony\Component\DependencyInjection\Exception\RuntimeException: Invalid constructor argument 7 for service "console.command.form_debug": argument 4 must be defined before. Check your service definition.
/app/src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php:48
/app/src/Symfony/Component/DependencyInjection/Compiler/Compiler.php:73
/app/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:813
/app/src/Symfony/Component/Form/Tests/DependencyInjection/FormPassTest.php:86
Do you have an alternative for that?
I do like the feature, but thinking about an extreme example of going all-in on attributes, I wonder were it goes. Small simple example of building an app where we store informations about Events sent by users with our own schema, and we also receive informations from APIs with their own structure (you see where this is going). So we need the usual ORM attributes, some Validation, Object-Mapper, and these Form attributes. The entity becomes quite heavy on attributes. use App\Dto\Event as EventDTO;
use App\Repository\EventRepository;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Form\Attribute\AsFormType;
use Symfony\Component\Form\Attribute\Type;
use Symfony\Component\Form\Extension\Core\Type as FormType;
use Symfony\Component\ObjectMapper\Attribute\Map;
use Symfony\Component\Validator\Constraints as Assert;
#[AsFormType]
#[Map(target: EventDTO::class)]
#[ORM\Entity(repositoryClass: EventRepository::class)]
class Event
{
#[ORM\Id]
#[ORM\GeneratedValue]
#[ORM\Column]
private ?int $id = null;
#[Map(target: 'eventName')]
#[Assert\Length(min: 15)]
#[Assert\NotBlank()]
#[ORM\Column(length: 255)]
#[Type]
private ?string $name = null;
#[Map(target: 'synopsis')]
#[Assert\NotBlank()]
#[ORM\Column(type: Types::TEXT)]
#[Type(FormType\TextareaType::class)]
private ?string $description = null;
// ... As you can see, the business need is small, there's not much in terms of validation or mapping, and I've included only few properties. I don't know if that's a problem honestly, it's not that unreadable to me, by I do think the question should be raised. That being said, once again, this is a really nice and neat feature, great job! |
I think in your usecase if you map object to a dto then the form should probably be on said dto. WDYT ? |
I know you love teasing me 😉 . Just to keep you happy:
I don't pretend this usecase is universale though. Once again, it's designed to create a fake extreme example, to show what it could lead. It's more of a "let's pretend there's a need like that". And then again, I don't say I'm definitely against this, I love the feature. But I also like to raise questions ;) |
This look like a fantastic addition for simple forms, but honestly I'm not 100% sure for mid-level and complex forms. PHP attributes are excellent for declaratively adding metadata, but there are limitations and cases where attributes are not the best fit. For instance, dynamic field definition, custom business logic, injecting services, or conditional configuration. In those cases, the traditional FormType classes may still be preferable (as they are services by definition). I don’t think we should replicate all form type capabilities with attributes (because the scope and dynamic limitations), but they can certainly be useful for building simple forms quickly, and then easily switch to FormType classes when things get more complex. |
@yceruto : Yes this is a totally valid point. I don't think attributes are meant to replace Forms. But should try to cover most common cases without the need to refactor it to a dedicated class. Might be worth adding a warning to the documentation of this feature the list of known limitations. |
$form = $formFactory->create(UserDTO::class, $user); It looks strange from the design PoV passing the DTO class as form type. I’m wondering if, instead of passing the data class, we can leave the form type param as $form = $formFactory->create(null, $user); // the form type will be guessed and defined from the DTO Maybe the same guesser mechanism could help with this? It'd be aligned with the null form type concept. |
@Tiriel Yes it can be used directly on entities, but even with your example, to make it more readable, a dedicated DTO for the form can still be done 😉 @yceruto I think its a good start to keep it simple yes. We just need to set the limit to which capabilities we want to replicate. I also thought of a command that can "convert" an attribute form to a traditional And Im totally agree to something like : $form = $formFactory->create(null, $user);
// or even?
$form = $formFactory->create($user); |
* @author Benjamin Georgeault <git@wedgesama.fr> | ||
*/ | ||
#[\Attribute(\Attribute::TARGET_PROPERTY)] | ||
final readonly class 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.
To stay consistent with the purpose of this attribute, as described in the class description, it represents a form field, so I’d prefer to name this class Field
?
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 Field
is wrong. Because it's biased by how it displays. But a "Field" could be an entire form. so I think we should keep Type
but maybe rename other occurences of Field's to Type's. WDYT ?
*/ | ||
public function __construct( | ||
private ?string $type = null, | ||
private array $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.
What about also adding ?string $name = null
, which defaults to the property name if null? If it’s set, it would use property_path for mapping. This could be convenient in cases where you need to change the field name in the view
In that case, what would be the benefit of using a new dedicated DTO+ attribute over a regular FormType class? |
There are implicit limitations with options and Closures, which may be mitigated in PHP 8.5, but still, adding large blocks of logic in attributes feels a bit messy to me. I think advanced features like data mappers, form events, and model/view transformers carry special complexity and responsibility tied to the form type itself, but I’m open to making them configurable using attributes, as long as we can also implement them outside the DTO. |
Yes, but what about just creating a command that generate the form type from a DTO 😅? |
even |
($resolver = $this->createMock(OptionsResolver::class)) | ||
->expects($this->once()) | ||
->method('setDefaults') | ||
->with([ | ||
'label' => 'Foo', | ||
]); |
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.
We could create an instance of OptionResolver
and make assertions later instead. It will ensure that only setDefaults
is called.
// Arrange
$resolver = new OptionResolver();
// Act
(new MetadataType($metadata))->configureOptions($resolver);
// Assert
$this->assertSame(['label' => 'Foo'], $resolver->resolve());
($metadata = $this->createMock(FormMetadataInterface::class)) | ||
->expects($this->once()) | ||
->method('getBlockPrefix') | ||
->willReturn('Foo'); |
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.
What about declaring an anonymous class implementing FormMetadataInterface
in setUp
to make these tests easier to read?
Symfony does not guarantee named arguments on methods at the moment. So maybe better not show this on documentation / example ? |
@Tiriel Still one class less, less code:
Yes, using the closure inside attributes in 8.5 can remove some limitations.
Agreed
Can still be done, those 2 features are not mutually exclusive 😄 |
I don't think we're talking about the same thing and we're going deep into things not strictly related to this feature. I don't see the point of the DTO in the first traditional case. And so, the number of classes is the same. But that's beside the point. I'm not against you or the feature, I'm just saying that there are use cases where this could lead to bloated entities. And I'm not sure that advising user to put the attributes on another unnecessary class is the best solution. But then again I seem to be the only one fearing that so maybe it's not that much of a concern. |
In terms of number of classes, yes. But to be fair, it’s not significantly less code. We’re going to inline the form type definition within the DTO class using attributes instead, so you’ll have to write extra code for that purpose. The comparison should be in terms of coding: |
I loved the auto form example, but I’m afraid it would work only as a sample. In practice, you’ll need to define at least the field type to match the property’s purpose. |
Form Attribute
This PR is about how to use PHP Attribute to build a Symfony Form.
Build your form directly on your data (e.g. on a DTO but not limited to it 😉).
Brainstormed with @Jean-Beru @Neirda24 and @smnandre
Description
What in mind when starting this:
FormType
classdata_class
mandatory but implicit)Data first, Form second
instead ofForm first, Data second
" quoted from @Neirda24 😄FormType
(easy because in the background, it is still aFormType
😉)Here what it looks like with a basic example:
Basic features (implemented in this PR)
Basic form
| - With Attributes
| - Classic equivalence (without attribute)
| - Variant 1, with one attribute per FormType (Not implemented)
Thought of that too, I like the readability but maybe too much work to maintain.
Still need a 'GenericType' to work with type that does not have attribute equivalence.
With validation
Work like a charm, without to do anything more to support it
Class inheritance
| - With attribute
| - Classic equivalence (without attribute)
Next features (WIP)
Here some DX example of how to implement next features from form component with attributes.
For Form Event and Data Transformer, can be more powerful with PHP 8.5 closures_in_const_expr in the future \o/
Form Event
| - Event on the root FormType
| - With Attributes
| - Classic equivalence (without attribute)
| - Event on a FormType's field
| - With Attributes
| - Classic equivalence (without attribute)
Data Transformer
| - DT on the root FormType
| - With attribute
| - Classic equivalence (without attribute)
| - DT on a FormType's field
| - With attribute
| - Classic equivalence (without attribute)
| - Variant 1 with DataTransformerInterface
Not a fan but.
| - Variant 2, with 2 attributes
Use 2 distinct attribute, one for model transformer, another for view transformer.
Form Type Extension
Auto Form \o/
Need to be discussed / Possible evolution
Here stuff that I am no satisfy for now and need to be discuss to find a better way to do it.
Autowire
| - In options
| - Event
| - Data Transformer
configureOption method
| - Varaint 1 (not fan of it)
| - Varaint 2 (love it but PHP 8.5)
Override getParent
Not sure if this means something, maybe not needed. What do you think?
Symfony Demo Application
with Form AttributeYou can try the basic examples of
AsFormType
in theSymfony Demo Application
fork:https://github.com/WedgeSama/demo-with-form-attribute/tree/demo-with-form-attribute
List of
FormType
replaced (examples on both DTO and entities):Form\ChangePasswordType
=> created DTODTO\UserChangePasswordDTO
Form\UserType
=> created DTODTO\UserProfileDTO
Form\CommentType
=> on existing entityEntity\Comment
Form\PostType
=> on existing entityEntity\Post
It use git submodule to require the form component with form attribute.
PS: any suggestion are welcome 😉