Skip to content
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

[Form] Ease immutable/value objects mapping #19367

Closed
wants to merge 1 commit into from
Closed

[Form] Ease immutable/value objects mapping #19367

wants to merge 1 commit into from

Conversation

ogizanagi
Copy link
Member

@ogizanagi ogizanagi commented Jul 15, 2016

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

by adding a new SimpleObjectMapperdata mapper and simple_object_mapper FormType option.

This new feature is based on the great @webmozart 's blog post about "Value Objects in Symfony Forms", but tries to expose a simpler API to ease value objects & immutable objects manipulations with Symfony forms.

Show code

Considering the following value object:

class Money
{
    private $amount;
    private $currency;

    public function __construct($amount, $currency)
    {
        $this->amount = $amount;
        $this->currency = $currency;
    }

    public function getAmount() // ...
    public function getCurrency() // ...
}

Before

class MoneyType extends AbstractType implements DataMapperInterface 
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('amount', NumberType::class)
            ->add('currency')
            ->setDataMapper($this)
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => Money::class,
            'empty_data' => null,
        ]);
    }

    public function mapDataToForms($data, $forms)
    {
        if (null === $data) {
            return;
        }

        $forms = iterator_to_array($forms);
        $forms['amount']->setData($data->getAmount());
        $forms['currency']->setData($data->getCurrency());
    }

    public function mapFormsToData($forms, &$data)
    {
        $forms = iterator_to_array($forms);

        // Logic to determine if the result should be considered null according to form fields data.
        if (null === $forms['amount']->getData() && null === $forms['currency']->getData()) {
            $data = null;

            return;
        }

        $data = new Money(
            $forms['amount']->getData(),
            $forms['currency']->getData()
        );
    }
}

After

class MoneyType extends AbstractType implements FormDataToObjectConverterInterface 
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('amount', NumberType::class)
            ->add('currency')
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => Money::class,
            'simple_object_mapper' => $this,
        ]);
    }

    /**
     * {@inheritdoc}
     *
     * @param Money|null $originalData
     */
    public function convertFormDataToObject(array $data, $originalData)
    {
        // Logic to determine if the result should be considered null according to form fields data.
        if (null === $data['amount'] && null === $data['currency']) {
            return null;
        }

        return new Money($data['amount'], $data['currency']);
    }
}

After (with shortcut)

with simple_object_mapper option as a callable

class MoneyType extends AbstractType 
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('amount', NumberType::class)
            ->add('currency')
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => Money::class,
            'simple_object_mapper' => function (array $data) {
                // Logic to determine if the result should be considered null according to form fields data.
                if (null === $data['amount'] && null === $data['currency']) {
                    return null;
                }

                return new Money($data['amount'], $data['currency']);
            },
        ]);
    }
}
Show basic API (without using the form option)
Setting the data mapper yourself:
class MoneyType extends AbstractType implements FormDataToObjectConverterInterface
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('amount', NumberType::class)
            ->add('currency')
            ->setDataMapper(new SimpleObjectMapper($this, $builder->getDataMapper()))
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefault('data_class', Money::class);
    }

    /**
     * {@inheritdoc}
     *
     * @param Money|null $originalData
     */
    public function convertFormDataToObject(array $data, $originalData)
    {
        // Logic to determine if the result should be considered null according to form fields data.
        if (null === $data['amount'] && null === $data['currency']) {
            return null;
        }

        return new Money($data['amount'], $data['currency']);
    }
}
Using the CallbackFormDataToObjectConverter:
class MoneyType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('amount', NumberType::class)
            ->add('currency')
            ->setDataMapper(new SimpleObjectMapper(new CallbackFormDataToObjectConverter(function (array $data, Money $originalData ) {
                // Logic to determine if the result should be considered null according to form fields data.
                if (null === $data['amount'] && null === $data['currency']) {
                    return null;
                }

                return new Money($data['amount'], $data['currency']);
            }), $builder->getDataMapper()))
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefault('data_class', Money::class);
    }
}

I make this suggestion in order to ease value objects & immutable objects manipulations in forms, considering the fact most of the time, we don't need to bother about implementing the DataMapperInterface::mapDataToForms() method. We can simply reuse the PropertyPathMapper implementation.

Then, only the DataMapperInterface::mapFormsToData() method is useful. But once again, we don't really need to deal with Form instances, only with form's data, in order to describe to our form type how to create a new value object instance.

For most advanced usages, an ObjectToFormDataConverterInterface interface can also be implemented, allowing to skip the original mapper (in most cases the PropertyPathMapper) implementation, to simply map the data to the form ourself, by converting to value object to an array of form data indexed by field name.

Show `ObjectToFormDataConverterInterface` implementation sample
class MediaConverter implements FormDataToObjectConverterInterface, ObjectToFormDataConverterInterface 
{
    // ...

    /**
     * {@inheritdoc}
     *
     * @param Media|null $object
     */
    public function convertObjectToFormData($object)
    {
        if (null === $object) {
            return array();
        }

        $mediaTypeByClass = array(
            Movie::class => 'movie',
            Book::class => 'book',
        );

        if (!isset($mediaTypeByClass[get_class($object)])) {
            throw new TransformationFailedException();
        }

        return array(
            'mediaType' => $mediaTypeByClass[get_class($object)],
            'author' => $object->getAuthor(),
        );
    }
}

Now, given the following two facts:

  1. There are only two hard things in Computer Science: cache invalidation and naming things.
  2. This PR does not deal with cache invalidation at all.

I suck at naming things, and I'll gladly consider any better name for the followings:

  • The 2 FormDataToObjectConverterInterface and ObjectToFormDataConverterInterface interfaces and methods.
  • The SimpleObjectMapper data mapper name and simple_object_mapper option.

Note: I originally made this available as a FormType extension for one of our projects. But considering the DDD and clear DTOs, Model & Entity separation vibes, I think it makes sense to have this new mapper in the Symfony\Form\Extension\Core namespace.


As a solution until a decision is made here, the code is available in a dedicated repository: https://github.com/Elao/FormSimpleObjectMapper

@linaori
Copy link
Contributor

linaori commented Jul 16, 2016

While I do not disagree (nor agree) with the PR, I would advise against Value Objects in forms, as much as Entities. A simple DTO which is allowed to be in an invalid state, is a lot easier and you can simply populate whatever data you need with the DTO. http://stovepipe.systems/post/avoiding-entities-in-forms

This requires no weird data object filling callbacks or what so ever to be achieved. I suggest people to keep it simple and keep it decoupled.

@ogizanagi
Copy link
Member Author

ogizanagi commented Jul 16, 2016

At first, we used simple DTO objects (with getter/setter or public properties), more by being constrained by Symfony forms & other Infrastructure stuff than by conviction.
It clearly keeps things easy and straightforward, without much efforts.
But in most cases, it wasn't satisfying nor it made sense to denature those objects which belonged to our Application namespace, only because of some Infrastructure constraints.

In our projets, we're using a lot the command bus, and we often delegate the creation of the Command/Query instance to form. Thus, forms are in charge to extract data from the request, create the command/query instance, and finally, validate it before passing it to the bus. The latter being in charge to handle the command and reflect the changes in our entities.
Thus, in our case, it only makes sense to have our objects in a valid state, with proper encapsulation, and having them as an immutable object brings even more control.

Value objects, on another hand, brings much more flexibility and reusability for simple objects across the application, and does not require any intermediate DTO, but can allow to directly map our model to the forms (encapsulated in a command or not). There is no such things as risky as with entities, because they are value objects, are not directly mapped to our entities, and are a strong echo to our model in each layer of the application.

Now, I agree with your comment, and I actually think both approaches are fully valid, but can be preferred according to different use cases, or simply by taste/conviction.

I hope this make sense to you and other folks and would help considering new features in this direction. 😃

Finally, I don't understand why you'd advise against Value Objects in forms as much as Entities ?

(BTW, I know about your blog post, and I'm 👍. Great post.)

@linaori
Copy link
Contributor

linaori commented Jul 16, 2016

Finally, I don't understand why you'd advise against Value Objects in forms as much as Entities ?

For the exact reason you've just mentioned:

Thus, in our case, it only makes sense to have our object in a valid state, with proper encapsulation, and having it as an immutable object brings even more control.

Take the following example of a Value Object: new Money('boo', 'far'); This is an invalid state, in theory this would throw an error (if you use php7) or userland exception from setting a non numeric / non-existing valuta value.

If you allow this, your Money is still in an invalid state and if you disallow this, your Money object will throw an exception, which means it won't even reach the validation part.

@ogizanagi
Copy link
Member Author

Take the following example of a Value Object: new Money('boo', 'far'); This is an invalid state, in theory this would throw an error (if you use php7) or userland exception from setting a non numeric / non-existing valuta value.

Transformers and specialized form types would not allow such a thing, and even if it was the case, you're able to throw a TransformationFailedException inside a data mapper.

@linaori
Copy link
Contributor

linaori commented Jul 16, 2016

Which would not be a graceful error handling. If you truly want your ValueObject to always be in a valid state, you can't use it with Symfony Forms, because an object that has to be validated can be in an invalid state.

Edit:
With graceful error handling, I mean violations to be added as error in the form.

@ogizanagi
Copy link
Member Author

ogizanagi commented Jul 16, 2016

Which would not be a graceful error handling

A TransformationFailedException is gracefully transformed into a form error.

If you truly want your ValueObject to always be in a valid state, you can't use it with Symfony Forms, because an object that has to be validated can be in an invalid state.

I don't get your point. Mapping an object that won't need to be validated itself by an external component (it may be validated according to the context, though) should not prevent me to use Symfony Forms. It's not the only data submitted.

But anyway, mine is not only about value objects, but about the fact I don't want either to design my commands and Application stuff according to Infrastructure limitations.
I won't advocate a solution over another one more than this. Being pragmatic and opt for simplicity is as valid to me as opting for the long way according to your own preferences. But if we're able to short cut a little the long way, and bring people some opportunity to decide which one to go with equal chances, I'd say let's do it.

...But of course, only if it makes sense to some majority 😄

@unkind
Copy link
Contributor

unkind commented Jul 17, 2016

http://stovepipe.systems/post/avoiding-entities-in-forms

@iltar ChangeUsernameData from your blog post is a command message. Command message should be immutable. From this point of view, command is a value object. At the same time, command is DTO. These 2 concepts are not mutually exclusive.

@ogizanagi
Copy link
Member Author

ogizanagi commented Jul 17, 2016

@unkind : Just to be clear about your pretty good observation: a command should be an immutable object. Not particularly a value object (which conveys an even stronger signification).

@unkind
Copy link
Contributor

unkind commented Jul 17, 2016

I consider command message as a particular case of VO.

We don't allow to create invalid VOs, because they don't make sense.
But "invalid" command messages make sense. It's OK if user wants to register with too long username. We allow to create command object, but we don't allow to pass its data to domain.
It's just a little bit different meaning of "invalid", IMO.

@ogizanagi
Copy link
Member Author

ogizanagi commented Jul 17, 2016

Interesting. That's exactly where I put the boundary (at least one of the hints) between a simple immutable object and a value object. 😅

To me, a value object is validated by itself, as a self-sufficient object and has a strong meaning about the value it encapsulates.
A simple immutable object can be created in an invalid state, but can (should!) be validated afterwards by an external service. This validation will prevent any invalid command to be handled and reflected in the domain. The immutability is here to preserve the integrity of the command message once validated.

So I guess we actually substantially agree, but do not share the exact same definition.

@unkind
Copy link
Contributor

unkind commented Jul 17, 2016

By analogy, you can compare command message with HTTP-request message.
HTTP-request is a value object (I hope we agree on this one).
HTTP-request can be valid by itself (HTTP-specification level), but can be "invalid" from controller point of view.
Command message is valid by itself as well (API-level; for example, we just require string, but we don't declare exact rules), but it can be "invalid" from validator point of view.

@linaori
Copy link
Contributor

linaori commented Jul 17, 2016

I do have to note that in the explanation I gave, you can compare the DTO to a Command; E.g. ChangeUsernameData or ChangeUsernameCommand would come down to the same thing. One of the reasons I call them "Data" instead of Command, is that it gets confusing for other developers with Symfony Command classes.

However, with (Symfony) forms, you want to add violations and not throw errors/exceptions. For example:

__construct($amount, $currency)
    if !is_number $amount: throw new InvalidArgumentException;

This would prevent the object from ever be in an invalid state (which is what you've indicated). If you were to simply accept every value in the constructor, that means your object will not be able to be "valid" in a way you can ensure it upon creation. This is what I mean with:

If you truly want your ValueObject to always be in a valid state, you can't use it with Symfony Forms, because an object that has to be validated can be in an invalid state.

If you throw an exception, you can't add it as error message for your form. If you don't throw the exception, it can be created in an invalid state.


As @unkind mentioned immutable objects, then it does make sense to solve it the way you propose. There's a big difference between an immutable object and an object that always has to be valid.

For me this method would be extra overhead. If I have a MoneyData object, I'll simply make a corresponding MoneyType which maps those fields and have a parent type containing that MoneyType It's up to that parent type to make sure there's always an object available.

I would also like to mention that as a DTO, I fill data in 2 places. As this object contextually transfers data, I also set data from the controller, for example the subject in question. This comes from the Action arguments for example, which went through authorization. If I have to add this contextually as subject, I have to call a setter, making it impossible to make this object immutable or always in a valid state. Or the other way around, my DTO cannot exist without a subject, thus I have to create the object with the subject as constructor argument.

My Form will make sure the DTO is validated and once done, my onSuccess path will grab that data and do whatever it needs; E.g. fill an entity or make an API call. (Yes I use a custom form handler to move this logic from the controller)


So, to make this story short, I think it's a lot of extra effort to create your objects this way. However, I do think there are use-cases if immutability works for both your forms and your default data. For objects that have to always be in a valid state, this won't work due to the DTO being in an invalid state from raw data.

@maryo
Copy link
Contributor

maryo commented Jul 17, 2016

I use command bus too. But my commands are actually just simple DTOs validated by validator component and mostly constructed and "filled in" by form component. They are not immutable.

"A Command is a message that represents the intention of the user. Command validation happens after the form is submitted and before the Command is passed to the model."

http://verraes.net/2015/02/form-command-model-validation/

OFC the commands could be immutable but then I couldn't easily use them as DTOs and would need to create an additional ones.
I use VOs in my command but I don't understand the command itself as a standard VO.
DTO and VO is not mutually exclusive by definition. But in my case it is since it's not immutable :)

In my controller I don't need to do any more logic than passing the command to command bus. And the commands have their intention mor clear in their name. For example RegisterUserCommand vs RegisterUserData.

And it's a little bit OT but in my value objects i throw validation exeptions (not TransformationFailedException , I've prepared a special interface for them - only for those in my domain) and I then catch it using a special form extension which translate the exception messages and fills the form errors based on those exceptions (you want to propagate only some of them) to the form.

I am +1 (but ok i am just an ordinary person passing by :)) for somehow ease immutable/value objects mapping, not sure if particulary for this PR, not against :). The current way of passing the data array by reference is a little bit weird. But at least it works. The callback option could be a way.

@unkind
Copy link
Contributor

unkind commented Jul 17, 2016

@iltar I agree with you on value objects like Money, I wouldn't map form on it directly either.
Commands is the only case that comes on my mind.

Moreover, if my API is command-oriented, I probably don't need the Form component at all.
I just need endpoint POST /command-bus/, deserializer and the Validator component (However, sometimes I need custom mapping between API-command and internal one, because they are not supposed to match one-to-one in general).

My message was about terminology. It looks like we have a little bit different opinions on whether command is a value object, but it's not that important.

@ogizanagi
Copy link
Member Author

ogizanagi commented Jul 17, 2016

Here is a test case showing how behaves value objects in forms if you're using proper form types for building the value object, or if you throw yourself a custom TransformationFailedException.

Now I'm not advocating about binding value object to your forms or not. I just say it is possible and can be gracefully handled.
But this PR targets both immutable objects and value objects since the beginning, and my own use cases are more related to immutable objects on which I'll apply validation rules afterwards. 😃

@HeahDude
Copy link
Contributor

@ogizanagi That's a nice feature! Thanks for proposing it in the core. I've already thought about using a DataMapper as a simple callable.

Especially after I've read a long time ago what makes me saying now that this option looks very close to the data_constructor concept (current empty_data) (ref #5939).

Shouldn't we use the same logic in both cases, whether submitted data is null or composed?

Actually the empty_data option looks like either unused, misused or bugged... but this would need a refactoring in Form::submit() too.

@ogizanagi
Copy link
Member Author

ogizanagi commented Jul 17, 2016

@HeahDude : Thanks for sharing your opinion on this. 😃

Shouldn't we use the same logic in both cases, whether submitted data is null or composed?

I'm not sure about what you meant ? If you actually meant the data_constructor option (so the empty_data) should have been used for both creating and editing objects according to submitted data, yes indeed. That's more or less the idea with this new data mapper, appart that it not only builds the object, but also and maps it to the form if an object was passed as initial form data.

Your reference makes me think I'd like the idea of having the simple_object_mapper option renamed to something like data_factory or data_constructor, and give the SimpleObjectMapper a DataFactoryInterface instance instead of a FormDataToObjectConverterInterface.
But then it would not really make sense to also accept an instance of ObjectToFormDataConverterInterface and will not convey a real meaning about what is actually happening behind. 😕

@chalasr
Copy link
Member

chalasr commented Jul 20, 2016

Not talking about the logic of performing validation on VOs via Forms but, the current way being quite annoying, the feature sounds really nice to me.
👍

Edit: If throwing a TransformationFailedException in a DataMapper makes the exception converted into a FormError, to me the error handling is indeed graceful and VOs can perfectly be always valid since you won't instantiate them if this exception is thrown

@ogizanagi
Copy link
Member Author

Hi @webmozart :)

May I ask you your opinion on this feature ?

by adding a new "simple_object_mapper" FormType option.
@ogizanagi
Copy link
Member Author

@HeahDude: Do you think this feature has any chance to have a future in the Symfony core or is it too marginal? Does it need more flexibility or anything to be enhanced?

@HeahDude
Copy link
Contributor

HeahDude commented Jan 21, 2017

Hey @ogizanagi, I understand the motivation behind this PR, and I'm not fundamentally against it (as I said before), but I think the current implementation could be simplified a lot; I don't think we really need new interface(s) to handle it for example.

Anyway before you keep working on this one, I'd like to sanitize the current implementation of the component first by refactoring some parts, and maybe solve this use case in the process.

Have you already read #8834?
Although I don't agree with every part of it (some things have changed since then), I think I'm getting somewhere, actually, my first step is to deal with #4102 which is somehow tied to #5939 (see my above comment) and that's why I'm saying I may solve your use case in the process.

So I propose to come back later on this, besides you made a bundle for it ;). So if anyone wants to use/test it, I guess he can :D.

@ogizanagi
Copy link
Member Author

Great. I'm looking forward to seeing what you're going to produce :)

@ogizanagi ogizanagi changed the base branch from master to 3.4 May 17, 2017 18:32
@nicolas-grekas nicolas-grekas removed this from the 3.4 milestone Oct 8, 2017
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Oct 8, 2017
interface FormDataToObjectConverterInterface
{
/**
* Convert the form data into an object.
Copy link
Contributor

@hhamon hhamon Nov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{@inheritdoc}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface does not extend any other interface.

interface ObjectToFormDataConverterInterface
{
/**
* Convert given object to form data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converts.

@fabpot
Copy link
Member

fabpot commented Mar 31, 2018

@HeahDude @ogizanagi What's the status here? Looks like @HeahDude was going to propose something, but...

@ogizanagi
Copy link
Member Author

I think we can close as it does not seem to get much attraction. If anyone is interested again, they can give a try to this package first. We can still consider reopening it later.

@javiereguiluz
Copy link
Member

@ogizanagi sounds like a good plan. Let's close then. Thanks!

@ogizanagi ogizanagi deleted the form/immutable_objects_mapper branch March 31, 2018 09:53
@ostrolucky
Copy link
Contributor

Delegating this to a package which is dead on arrival is odd though.

But yeah, at least you can take a peek and copy things you need out of it.

@ogizanagi
Copy link
Member Author

ogizanagi commented Apr 1, 2018

at least you can take a peek and copy things you need out of it.

Or provide a PR to update the Sf compatibility ;)
But fair, I'll update it soon (EDIT: done).

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.

None yet