Skip to content

[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

Closed
wants to merge 3 commits into from

Conversation

alcaeus
Copy link
Contributor

@alcaeus alcaeus commented Jul 20, 2020

Q A
Branch? 5.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #37597
License MIT
Doc PR symfony/symfony-docs#...

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 a setName call (that the PropertyAccess component will seek out) to the rename method in the Category entity.
A more complex solution uses the EditCategoryTwoStep DTO, which contains public properties for all fields, then maps them to the Category entity in a special apply 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 before apply 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:

final class Category
{
  private string $name;

  public static function fromName(string $name): self
  {
    return new self($name);
  }

  private function __construct(string $name)
  {
    $this->name = $name;
  }

  public function getName(): string
  {
    return $this->name;
  }

  public function rename(string $name): void
  {
    $this->name = $name;
  }
}

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:

$builder->add('name', null, ['set' => function(Category $category, string $name) { $category->rename($value); }]);

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:

final class Category
{
  private string $name;

  public static function fromName(string $name): self
  {
    return new self($name);
  }

  private function __construct(string $name)
  {
    $this->name = $name;
  }

  public function getName(): string
  {
    return $this->name;
  }

  public function rename(string $name): self
  {
    return self::fromName($name);
  }
}

Again, the form framework does not support this. With accessors, this becomes increasingly simple:

$builder->add('name', null, ['set' => function(Category $category, string $name) { return $category->rename($value); }]);

If the setter returns a non-null value, the mapper behaves as follows:

  • If the original data is a scalar value, a scalar value replaces the form data. Otherwise, it is discarded.
  • If the original data is an array, returning another array also replaces the form data. Otherwise, it is discarded.
  • If the original data is an object, and the returned object is an instance of the same class, this replaces form data. Otherwise, it is discarded.

The form data is overwritten after each step, so any subsequent calls to accessors already operate on the new object.

Basic validation

Any TypeError or Symfony\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):

$builder
    ->add('address', AddressType::class, [
        'read_property_path' => 'shippingAddress',
        'write_property_path' => 'ship',
    ])
    ->add('trackingNumber', TextType::class, [
        'read_property_path' => 'trackingNumber',
        'write_property_path' => 'ship',
    ])
 ;

With closures being used for accessors, this is no longer feasible. It is thus suggested to set your own set accessor:

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        parent::buildForm($builder, $options);

        $builder
            ->add('address', AddressType::class)
            ->add('trackingNumber', TextType::class)
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        parent::configureOptions($resolver);

        $resolver->setDefault('set', function (Order $order, array $data) { $order->ship($data['address'], $data['trackingNumber']);
    }

This use case is also tested in the testSetUsesAccessorForCompoundFields test for the access mapper itself.

@julienfalque
Copy link
Contributor

This looks very interesting! This is kind of similar to something I proposed for the serializer (#37419).

Any TypeError or Symfony\Component\Form\Exception\ExceptionInterface from a 'set' accessor are caught and the exception message added as a form error.

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 TypeError would not cover all the cases as most invariants violations are reported using e.g. InvalidArgumentException. Symfony\Component\Form\Exception\ExceptionInterface could be useful but one might prefer not having its domain code depend on it.

@alcaeus
Copy link
Contributor Author

alcaeus commented Jul 20, 2020

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()) {
Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 22, 2020

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

success-handler => fn (FormData $data) {
    $dto->setField($data->getModelData('field'));
    $data->setFieldsAutomatically($dto, ['other-field']);
}
error-handler => fn() {}

(FormData to retain access on both view- and model-data)

@alcaeus
Copy link
Contributor Author

alcaeus commented Jul 23, 2020

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.

You are right, but that's already the case, it's just hidden from view: 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). Think of this PR as making these rules more explicit: your form will always have to know how to map its data back to yours, unless you want to do that yourself completely (e.g. in an apply method of a DTO). Even in that case, the form still would have to know about how to bind data to your DTO.

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.

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
[...]

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.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 23, 2020

each individually field knowing about mapping rules.

but that's already the case

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 :)

the form needs to know about the DTO

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 👍

@alcaeus
Copy link
Contributor Author

alcaeus commented Jul 23, 2020

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!

Copy link
Member

@yceruto yceruto left a 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());
Copy link
Member

@yceruto yceruto Jul 24, 2020

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?

Copy link
Member

@yceruto yceruto Jul 24, 2020

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
Copy link
Member

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()) {
Copy link
Member

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).

Copy link
Member

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.

Copy link
Contributor Author

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.

@yceruto
Copy link
Member

yceruto commented Jul 25, 2020

As for the other examples, I always encounter people suggesting building your own data mapper. In my experience, this is quite cumbersome

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)) {
Copy link
Member

@yceruto yceruto Jul 25, 2020

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); 
    },
]);

Copy link
Contributor Author

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.

@yceruto
Copy link
Member

yceruto commented Aug 27, 2020

Hi @alcaeus, I took over your proposal with a different implementation, but same goal ! #37968
I've taken some ideas from your comments,
Thank you a lot.

@fabpot
Copy link
Member

fabpot commented Sep 2, 2020

@alcaeus Can you have a look at @yceruto proposal and see if that could replace your PR here? Thank you.

@alcaeus
Copy link
Contributor Author

alcaeus commented Sep 2, 2020

Closing as the approach by @yceruto makes this a core concept of the forms instead of just tacking it on via an extension.

@alcaeus alcaeus closed this Sep 2, 2020
@alcaeus alcaeus deleted the form-accessors branch September 2, 2020 15:13
fabpot added a commit that referenced this pull request Sep 16, 2020
…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:

![data_accessor](https://user-images.githubusercontent.com/2028198/91452859-16bf8f00-e84d-11ea-8564-d497c2f73730.png)

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
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] [Form] Improve handling of domain objects in forms
8 participants