-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[POC] [Form] Support get/set accessors for form fields #37614
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
This looks very interesting! This is kind of similar to something I proposed for the serializer (#37419).
What do you think about making the classes of caught exceptions configurable somehow? The use case would be the same as in my proposal: rely on invariants validation from Value Objects to validate input instead of duplicating those rules in the Validator mapping. I think we could completely avoid using the Validator in simple scenarios but I'm afraid |
Configurable exception handling is on the to-do list. For the time being I used a pre-existing exception interface. However, users may want to extend or implement a generic DomainException that is then handled. I prefer this expressive usage of an exception interface over another form option. |
|
||
// Write-back is disabled if the form is not synchronized (transformation failed), | ||
// if the form was not submitted and if the form is disabled (modification not allowed) | ||
if (null !== $this->set && $config->getMapped() && $form->isSubmitted() && $form->isSynchronized() && !$form->isDisabled()) { |
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 shouldn't be possible for $this->set
to be null
as you checked it a few lines earlier (to fallback on the old datamapper).
Unless I read something wrong ?
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 are correct. I'll update this.
what generally bugs me, still, is each individually field knowing about mapping rules. Either on the compound form (default option), or per child form (given option). This increases complexity IMHO, as hydration can - and probably will - scatter around the codebase, making it hard to grasp what's going on, on high level. This is why i ultimately think we should reduce complexity in forms by a lot, letting the user hydrate as nessecary - in valid state, thus more or less
(FormData to retain access on both view- and model-data) |
You are right, but that's already the case, it's just hidden from view: the What I'm suggesting is that we put this coupled logic together: the form needs to know about the DTO, so it makes sense that this kind of mapping logic is defined in the form. As for the other examples, I always encounter people suggesting building your own data mapper. In my experience, this is quite cumbersome: public function mapFormsToData(...)
{
foreach ($forms as $form) {
switch ($form->getName()) {
case 'name':
$data->rename($form->getValue());
break;
case 'visible':
$data->makeVisible();
break;
default:
// Exception?
}
}
} This is not very practical, regardless of whether you need to change behaviour for a single field or for multiple fields.
Your example requires you to write an apply method that applies form data to your DTO or entity. It would also require you to pass a handler that initialises it, essentially forcing everyone to inline their data mappers. Again, this is not very practical, as many forms will be rather straightforward. |
true, but it's, IMHO, what causes the form complexity in general. It's over-abstract if you ask me, somewhat a framework on itself :)
ultimately i disagree, the user knows about the DTO/subject in relation to a/its form. And it's matter of composition basically, glueing independent concerns. Note, i wont block the PR (not that i can anyway :P); i think it's a good improvement from the current design pov overall 👍 |
Thank you! I'm planning to make some more improvements to forms with respect to DTOs, so your input is always welcome! |
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.
Thank you for proposing it, this seems a very interesting feature. Although I would use it for single fields only rather than compound ones. For compound forms I'm not sure what would be the advantage compared to DataMapperInterface
.
// if the form was not submitted and if the form is disabled (modification not allowed) | ||
if (null !== $this->set && $config->getMapped() && $form->isSubmitted() && $form->isSynchronized() && !$form->isDisabled()) { | ||
try { | ||
$returnValue = ($this->set)($data, $form->getData()); |
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.
Given a compound form with two fields (let's say name
and active
, string
and bool
respectively) If I understand it correctly, the same 'set' accessor will be used for all (children) form data, is that expected? shouldn't the 'set' accessor of the current child be called instead?
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.
Basically, I expected something like this $set = $form->getConfig()->getOption('set')
somewhere. same for 'get' accessor.
use Symfony\Component\Form\FormBuilderInterface; | ||
use Symfony\Component\OptionsResolver\OptionsResolver; | ||
|
||
class AccessorMapperExtension extends AbstractTypeExtension |
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.
Missing service registration in FrameworkBundle/Resources/config/form.php
return; | ||
} | ||
|
||
if (!$dataMapper = $builder->getDataMapper()) { |
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.
By default, there is no data mapper bound to single form fields (see FormType
, only compound forms will have a default data mapper), so the accessors configured for individual fields wouldn't work unless you call them from AccessorMapper
, but that's not the case currently (related to previous comment).
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.
The chicken and egg problem, if the parent (compound) form doesn't configure any accessor option then AccessorMapper
won't be set.
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.
Ah, yes. I'll add some functional tests to cover this as well.
Please note that this can be done in another way: public function mapFormsToData(iterable $forms, &$category)
{
$forms = iterator_to_array($forms);
$category->rename($forms['name']->getData());
if ($forms['visible']->getData()) {
$category->makeVisible();
}
// ...
} Very similar to what you proposed to do with 'set' accessor for compound forms. |
if ( | ||
(is_scalar($data) && gettype($data) === $type) | ||
|| (is_array($data) && is_array($returnValue)) | ||
|| (is_object($data) && $returnValue instanceof $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.
[Immutability] We could also apply the same approach as data mappers but on the closure &$data
and get rid of extra checks and rules in our side, so it's the user responsability and it'll work same:
$builder->add('name', null, [
'set' => function(Category &$category, string $name) {
$category = $category->rename($name);
},
]);
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.
Interesting idea. Passing the argument by reference also makes it clearer that you are always modifying the data directly, apart from being consistent with the data mapper API.
Closing as the approach by @yceruto makes this a core concept of the forms instead of just tacking it on via an extension. |
…tions (yceruto) This PR was merged into the 5.2-dev branch. Discussion ---------- [Form] Add new way of mapping data using callback functions | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | Fix #37597 (partially) | License | MIT | Doc PR | symfony/symfony-docs#14241 Replaces #37614 ## What this solves Objects and Forms have different mechanisms for structuring data. When you build an object model with a lot of business logic it's valuable to use these mechanisms to better collect the data and the behavior that goes with it. Doing so leads to variant schemas; that is, the object model schema and the form schema don't match up. You still need to transfer data between the two schemas, and this data transfer becomes a complexity in its own right. If the objects know about the form structure, changes in one tend to ripple to the other. Currently, the Data Mapper layer separates the objects from the form, transfering data between the two and also isolating them from each other. That's fine, but at present the default data mapper has a limitation: _it's very tied to one property path_ (see [`PropertyPathMapper`](https://github.com/symfony/symfony/blob/5.1/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php)). That said, you'll have to write your own data mapper in the following situations: * When the property path differs for reading and writing * When several form fields are mapped to a single method * When you need to read data based on the model's state * When the mapping of the model depends on the submitted form data * ... Also, when we create a new data mapper, we usually forget about checking the status of the given data and forms. Whether the data is empty or not; throw an exception if the given data is not an object/array and whether the form field is submitted/synchronized/disabled or not. Not doing that could lead to unwanted behavior. ## What this proposes Create a new way to write and read values to/from an object/array using callback functions. This feature would be tied to each form field and would also mean a new way of mapping data, but a very convenient one, in which it won't be necessary to define a new data mapper and take into account all what it would imply when you only need to map one field in a different manner or perhaps in only one direction (writing or reading the value). This PR adds two new options for each form type: `getter` and `setter`, allowed to be `null` or `callable`: ```php $builder->add('name', TextType::class, [ 'getter' => function (Person $person, FormInterface $form): string { return $person->firstName().' '.$person->lastName(); }, 'setter' => function (Person &$person, ?string $name, FormInterface $form): void { $person->rename($name); }, ]); ``` This would give us the same possibilities as data mappers, but within the form field scope, where: * `$person` is the view data, basically the underlying data to the form. * `$form` is the current child form that is being mapped. * `$name` is the submitted data that belongs to that field. These two callbacks will be executed following the same rules as for property paths before read and write any value (e.i. early return if empty data, skip mapping if the form field is not mapped or it's disabled, etc). ## What this also proposes I based the implementation on solving this problem first: > #37614 (comment) > [...] the property_path option defines the rules on how it's accessed. From there, the actual way it is accessed (direct property access, accessors, reflection, whatever) are hidden from view. All that matters is the property path (which is deduced from the name if not explicitly set). [...] So splitting the default data mapper `PropertyPathMapper` into two artifacts: "[DataMapper](https://github.com/yceruto/symfony/blob/data_accessor/src/Symfony/Component/Form/Extension/Core/DataMapper/DataMapper.php)" and "[DataAccessor](https://github.com/yceruto/symfony/blob/data_accessor/src/Symfony/Component/Form/DataAccessorInterface.php)" would allow us adding multiple data accessors along the way (the code that varies in this case) without having to reinvent the wheel over and over again (the data mapper code). You can also think about a new `ReflectionAccessor` for instance? or use this `CallbackAccessor` to map your form partially from an external API? yes, you could do it :) Here is a view of the proposed changes:  Where "DataMapper" will take care of common checks, iterates the given child forms, manages the form data and all what is needed for mapping a standard form, whereas "DataAccessor" will take care of how to read and write values to/from the underlying object or array. ## BC The `PropertyPathMapper` is being deprecated in favor of `DataMapper` class, which uses the `PropertyPathAccessor` by default. Although `DataMapper` is now the default for each compound form, the behavior must remains the same (tests prove it). So that if `getter` or `setter` option is null (they're by default) the `CallbackAccessor` will falls back to `PropertyPathAccessor` either for reading or writing values. --- Sorry for the long description, but I think that sometimes it is necessary for too complex issues and big changes. Besides, now you know a little more about what these changes is about. /cc @xabbuh as creator of [rich-model-forms-bundle](https://github.com/sensiolabs-de/rich-model-forms-bundle) and @alcaeus as you had worked on this issue. WDYT? Commits ------- 878effa add new way of mapping data using callback functions
This is a proof of concept PR based on the original idea in the RFC above, but already incorporating feedback from the community.
Brief overview
This adds a new form type extension that defines 'get' and 'set' options for forms. If provided, the extension wraps the existing data mapper (usually a PropertyPathMapper) in an AccessorMapper with the provided accessors. If an accessor is left undefined, this falls back to the wrapped data mapper for compatibility.
Why?
To explain why we do this, let's consider the following gist: https://gist.github.com/alcaeus/0b66968ee505ab24bdb71fe7c88325a7
The
Category
entity has a non-standard accessor to rename the category. While this is expressive, it does not play nice with the form component out of the box. To remedy this problem, one could use the RichModelFormsBundle, which allows for changing the write access property path separate from the read property path.Alternatively, one can create a
EditCategoryProxy
DTO, which maps asetName
call (that the PropertyAccess component will seek out) to therename
method in theCategory
entity.A more complex solution uses the
EditCategoryTwoStep
DTO, which contains public properties for all fields, then maps them to theCategory
entity in a specialapply
method which needs to called after the form has been validated. The advantage of this is that it does not leave an entity in an invalid state, since constraints on the DTO are evaluated beforeapply
is called (typically by the user before flushing the entity to the manager). It also reduces boilerplate code by doing away with setters and getters and using public properties (since it's just a data holder).Both DTOs have a significant problem in my view: they are tightly coupled to a specific form, but are separate classes that contain lots of usual boilerplate code. This is where the addition of accessor closures comes into play.
Basic usage
Forms where you don't specify an accessor (all current forms) behave exactly like before, with no code being changed. They still use a PropertyPathMapper or whatever the user provided. However, upon specifying either 'get' or 'set' option causes cool things to happen. For all examples, please consider the following entity:
This class has no typical
setName
accessor as the property access component would expect, so using this entity in a form was previously coupled to using a DTO or a custom data mapper on the embedding form to handle this API. With form accessors, the form is defined like this:In this case, getting the value is handled through the PropertyPathMapper, while the value is changed using our closure.
Advanced usage
Apart from this basic usage, the mapper contains a few features that are nice for people who are more familiar with complex and rich models.
Using immutable objects
A typical example is storing dates, as it's recommended to always handle
DateTimeImmutable
objects. These return a new instance of themselves with the value applied. The old object is discarded or can be kept for auditing purposes. Let's change the category we had above to being immutable:Again, the form framework does not support this. With accessors, this becomes increasingly simple:
If the setter returns a non-null value, the mapper behaves as follows:
The form data is overwritten after each step, so any subsequent calls to accessors already operate on the new object.
Basic validation
Any
TypeError
orSymfony\Component\Form\Exception\ExceptionInterface
from a 'set' accessor are caught and the exception message added as a form error. This causes the form to fail validation before any changes are applied. However, the exception does not stop the processing of form fields, so this can serve as a sanity check before doing more significant validation (e.g. uniqueness checks).Compound fields
Example: we have a checkout form that contains address fields. The fields are added as single form fields, but should all map to a single accessor. The RichModelFormsBundle mentioned in the RFC makes this possible by defining the same
write_property_path
multiple times (from the documentation):With closures being used for accessors, this is no longer feasible. It is thus suggested to set your own set accessor:
This use case is also tested in the
testSetUsesAccessorForCompoundFields
test for the access mapper itself.