codedmonkey commented 6 years ago

I'm proposing the creation of a make:form-data command to create a Data Transfer Object, specifically for use with the Form component, that holds the data of your domain model (e.g. entities) when using a form type to modify it.

Some people argue that an entity should never be in an invalid state (for example, required field should always be filled) and this position has been strengthened due to the fact that typehinting in PHP7 can enforce this notion where public function getUsername(): string can't return null.

This is relevant because when interacting with an entity, the Form component calls getUsername() to fill the initial form value of the username field, so using a form type directly on a new entity will result in a PHP error that getUsername() cannot return null. A DTO solves this problem by carrying the data for such an entity with seperate typehinting to comply with those interaction requirements. This DTO can be validated before the entity is created, meaning the developer can prevent creating an entity with an invalid state in this way.

Using DTOs in forms isn't mentioned in the docs anywhere, so that might need to change first (see https://github.com/symfony/symfony-docs/issues/8893).

Example Interaction

$ php bin/console make:form-data

 The name of the form data class (e.g. GentleJellybeanData):
 > UserData

 The name of Entity or custom model class that the new form data class will be bound to (empty for none):
 > User

 created: src/Form/UserData.php

// src/Entity/User.php

/**
 * @ORM\Entity(repositoryClass="App\Repository\UserRepository")
 */
class User
{
    /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     */
    private $id;

    /**
     * @ORM\Column(type="string", length=255)
     */
    private $username;

    /**
     * @ORM\Column(type="string", length=255)
     */
    private $email;

    /**
     * @ORM\Column(type="string", length=255)
     */
    private $password;

    public function getId()
    {
        return $this->id;
    }

    public function getUsername(): string
    {
        return $this->username;
    }

    public function setUsername(string $username): self
    {
        $this->username = $username;

        return $this;
    }

    public function getEmail(): string
    {
        return $this->email;
    }

    public function setEmail(string $email): self
    {
        $this->email = $email;

        return $this;
    }

    public function getPassword(): string
    {
        return $this->password;
    }

    public function setPassword(string $password): self
    {
        $this->password = $password;

        return $this;
    }
}

Expected Result

Mainstream

In my own experience, I've rarely seen DTOs being more than carriers of data, which would mean one file will be generated, but other components of the project need to be reworked to fill and extract data in the DTO (like the Controller displaying the form):

<?php

namespace App\Form;

class UserData
{
    /**
     * @var string|null
     */
    private $username;

    /**
     * @var string|null
     */
    private $email;

    /**
     * @var string|null
     */
    private $password;

    public function getUsername(): ?string
    {
        return $this->username;
    }

    public function setUsername(?string $username): self
    {
        $this->username = $username;

        return $this;
    }

    public function getEmail(): ?string
    {
        return $this->email;
    }

    public function setEmail(?string $email): self
    {
        $this->email = $email;

        return $this;
    }

    public function getPassword(): ?string
    {
        return $this->password;
    }

    public function setPassword(?string $password): self
    {
        $this->password = $password;

        return $this;
    }
}

Opinionated

Another (likely more opininated) way of handling DTOs is to add the ability to fill and extract data directly into the DTO. In addition to the generated code above, the following code would also be generated:

<?php

namespace App\Form;

use App\Entity\User;

class UserData
{
    // fields

    public function __construct(User $user = null)
    {
        if ($user) {
            $this->extract($user);
        }
    }

    // getters / setters

    public function fill(User $user)
    {
        $user->setUsername($this->getUsername());
        $user->setEmail($this->getEmail());
        $user->setPassword($this->getPassword());
    }

    private function extract(User $user)
    {
        $this->setUsername($user->getUsername());
        $this->setEmail($user->getEmail());
        $this->setPassword($user->getPassword());
    }
}

in this way, the developer can interact with the DTO with just a few calls:

// New user form
$data = new UserData();

$user = new User();
$data->fill($user);

// Edit user form
$data = new UserData($user);

$data->fill($user);

javiereguiluz commented 6 years ago

To allow us make a better decision, could you please show an example of how would you run this command and the code it would generate? It'd be great if you could use the Symfony Demo app as an example because that way we can compare "the new way" with "the old way" of doing things. Thanks!

weaverryan commented 6 years ago

Yea, @javiereguiluz has a great idea. I'd like to see some proposed code. Definitely not against this :).

codedmonkey commented 6 years ago

Good suggestion, I tried to make a good example of what I'm thinking of in the original post, let me know if anything is still unclear.

mu4ddi3 commented 6 years ago

Does anything moves with this topic?

vudaltsov commented 6 years ago

I'd suggest using App\Form\Data namespace.

bogdaniel commented 6 years ago

Great idea would save us some time. :-).

ckrack commented 6 years ago

It might be easier to have App\Form\FoobarType and App\Form\FoobarData in the same directory. I'd extend make:form with a question (Do you want to generate a data object for this entity?). The generation could mostly be copied from make:form and make:entity. Or we could just copy and rename the entity and add the fill method.

Basically we should have an entity that just defines the variables + getters/setters and a DTO with all the assertions.

vudaltsov commented 6 years ago

@ckrack , I usually have structure like

App\Form\Data\*Data.php
App\Form\EventListener\*Listener.php
App\Form\Type\*Type.php
App\Form\TypeExtension\*TypeExtension.php.

On my experience it is pretty convenient in a huge project.

ckrack commented 6 years ago

@vudaltsov While I do agree, it's fairly easy to move the files where one wants them. It could be easier for a start, to put them in the same place. This should be dependent on where the team wants to go with makers. Keeping it simple and functional or enforcing best practices?

ckrack commented 6 years ago

@javiereguiluz @weaverryan I have setup a repo with some example code.

I used code from the Doctrine/EntityRegenerator.php to copy the setters/getters and added a class template. Right now this is just a separate maker, not integrated with make:form.

You can see the result in the repo, too:

gundamftw commented 5 years ago

Hi, what if I have fields that were generated from an entity class? For example, I have a Book Entity with properties id and name. And in the form builder I have it like

$builder->add('book', EntityType::class, [ 'class' => Book::class 'label' -> 'name']

What do I need to check in the data class for this field? @var Book or @var string?

zmitic commented 5 years ago

@codedmonkey I like the idea but in the meantime, take a look at https://github.com/sensiolabs-de/rich-model-forms-bundle

It is absolutely amazing bundle, mostly because it allows you to always have valid entity; dependencies are easier to inject in constructor than using 'empty_data' and TypeError exceptions are converted to validation errors.

The problem with DTO is excess code and if you work with entity collections, you are in trouble. I tried, I failed, that's why I am promoting this bundle. And I don't feel bad about that :smile:

u03c1 commented 2 years ago

I like the idea and think that we should encourage to separate data representation used for example in FormType, from the Entity class which should represent DB model. FormType is not always a 1to1 representation of the model, and this is why we have DTO or dataTransformer.

We see some ticket like "https://github.com/symfony/maker-bundle/issues/1175", and even if the purpose to be "as friendly as possible" is understandable, it's seems inconsistent that the ORM annotation/attribute claims that a field is not null-able, but the property claims the opposite.

Morever, using some tools like phpstan complains with errors: " Property App\Entity\Foo::$name type mapping mismatch: property can contain string|null but database expects string". It's seems a mistake to just "silenced" those error and allowed invalid state for a while.

TLDR: Entity should be valid in the context of DB, and create a DTO in the same time as the Entity from the maker:bundle would be a great feature.

zmitic commented 2 years ago

TLDR: Entity should be valid in the context of DB, and create a DTO in the same time as the Entity from the maker:bundle would be a great feature

@u03c1

This would work only for simple scalar fields, not for collections or multiple: true cases.

Here is why:

let's say you have m2m relation between Product and Category
you want multi-select of categories in your form (EntityType, multiple: true)
when you submit the form, this is what happens:
- Symfony will first read original categories
- then it will find submitted categories
- it will then compare these 2 arrays and make appropriate calls to addCategory and removeCategory on Product entity

This means that if you don't change anything, Symfony will not even make a single call to adder/remover. Symfony can compare these 2 arrays as Doctrine has identity-map and you will never get 2 different instances representing same row.

But: if you wrap these entities in some sort of DTO, you now have to do this comparison by yourself. I assure you, it is far from trivial job; you can't use simple == comparison, and PHP doesn't have ComparatorInterface to help you.

So what would happen, in best-case scenario, are unnecessary calls to adder/remover. Sure: Doctrine would delete old and add new ones, pretty much leaving things as they were.

But this is a big problem: plain m2m relations are rare, there are always some extra columns. In above example, imagine that instead of real m2m, you have 2 of one2many relations like described here.

So you would have new entity like

class ProductCategoryReference
{
    public function __construct(
        private Product $product,
        private Category $category,
        private DateTime $joinedAt = new DateTime(),
    ){}
}

Back to entity, fake generics for readability:

class Product
{
        private Collection<ProductCategoryReference> $categoryReferences;

        public function getCategories(): array<Category>
        {
            return $this->categoryReferences->map(fn($ref)=> $ref->getCategory())->toArray();
        }

        public function addCategory(Category $category): void
        {
            $ref = new ProductCategoryReference($this, $category);
            $this->categoryReferences->add($ref);
        }
}

By working with entities directly, you can change only these getters/adders/removers in one place (bound entity) but everything else in the code remain unchanged; getCategories() will still return array of Category entity, but in a slightly different way. Symfony will not make unnecessary calls to adder/remover and thus, the value of $joinedAt will remain as it was.

I did many of these before but not as simple as shown here. I even have collections having their own collections :smile:

Idea of not binding entities to forms make sense, but it is simply not worth pursuing; very little benefits, but big PITA for anything but the most simplest of forms.

u03c1 commented 2 years ago

@zmitic Thanks for your detailed answer !

Sure that collection is not trivial, but I'm not convinced that because dealing with collection is tricky, we shoud have a one size fit all Entity that will "represent the DB Model... but in fact not really". I'm in favor that "default" should be "Entity is always valid" and "Entity is only DB", but I don't really care if we can have an option in the maker.

As i said, if you use symfony to deliver your frontEnd with formType, at some point you will have some forms field that will not stick to you Entity.

So you will add some "mapped=false" field for this "lovely JS component", or a dataTransformer and that could be crappy or quiet challenging.
Then you will have some partial form, and you will start to add tons of conditional statement, or creating different formType for all cases (Am I editing, creating ? Am i an Admin ?), so your Entity = formType is not true anymore
Then because you have partial forms, and some edge case, you will add some conditional asserts (ex: As a user i can populate this field with 400 chars max, but an admin could add more). And you will have to decide to put the assert on the Entity ? On the FormType ? Both ?
Then you will add an API endpoint, and the flat body JSON will not be in sync with the entity at all, or you don't want to expose everything. You need to add some serialization groups and your entity is now a big bloated file. You will have to move back some assert to the entity, or put this in a some layer between your controller and the entity... or you may use a DTO ?

So IMHO, collection is a real thing, but in a medium to large project, this will not be the biggest issue, and the database will drastically diverge from the UI/Entrypoint. At that time, I think DTO can clearly separate the view from the model and allowing more flexibility.

zmitic commented 2 years ago

First: sorry to all subscribers that will get emails. I just want to give real life examples why I think binding DTOs to forms is bad.

@u03c1

I use everything you mentioned, but not with default Symfony mapper. Instead, I use my own which is very similar to this one. The difference is my version doesn't have catch(TypeError); it is up to user to take care about that. And few minor things like order of params in callback, adding extra validation based on typehints... things like that.

Both of them allows users to not work directly on underlying entity, but use other entities and even some API if needed. I have cases like that but they are kinda tricky to show w/o understanding the reasons for that.

But: the comparison of what it was, to what is submitted is still a really big problem; I don't want my setters/adders/removers to be called for nothing which they would with DTO.

Now... there is one thing I didn't mention, but it is good you did. Symfony has the concept of parent forms so this is real code I have (slightly modified for readability):

/**
 * @extends AbstractType<Patient>
 */
class PatientFactoryType extends AbstractType
{
    public function __construct(private PatientFactory $factory) { }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefault('factory', $this->factory->create(...));
    }

    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder->add('firstName', TextType::class, [  // <----- field name DOES NOT have to match property name
            'get_value'    => fn(Patient $patient) => $patient->getFirstName(),
            'update_value' => fn(string $firstName, Patient $patient) => $patient->setFirstName($firstName),
        ]);
        // other REQUIRED values
}

This above form type is the absolute bare minimum for Patient entity to be created:

class Patient
{
    /**
     * @see PatientFactory::create()
     *
     * @internal
     * @psalm-internal \App\Factory\Entity\Strategy
     */
    public function __construct(
        private string $firstName,
        private string $lastName,
        private DateTimeInterface $dateOfBirth,
        private GenderEnum $gender,
        private Brand $brand,

        private ?string $phoneNumber = null,
        // other fields that are nullable are not shown
    ){}

Those other fields are populated by forms that extends the first one, which is the case you mentioned:

/**
 * @extends AbstractType<Patient>
 */
class PatientRegistrationType extends AbstractType
{
    public function getParent(): string
    {
        return PatientFactoryType::class;  // <---------------- this 
    }

    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder->add('phone', PrettyTelType::class, [
            'get_value' => fn(Patient $patient) => $patient->getPhoneNumber(),
            'update_value' => fn(string $phone, Patient $patient) => $patient->setPhoneNumber($phone),
        ]);

        // other fields
    }

With this getParent method, there is no duplication, no type errors and optional fields can be populated by other forms. By working directly on entity, underlying or not, calls to get*/add*/remove* will not occur if fields hasn't been submitted, or is the same as before.

Hope it is more clear now. Symfony forms are just crazy powerful, and I yet have to find a case when they can't do something. I even used them for API, no problems there; yes, you read it right, even for API :smile:

Anyway, I don't want this issue to become a chat as every other subscriber will get tons of emails. We can continue on /r/symfony , I have much more examples.

symfony / maker-bundle

[RFC] Add a form DTO maker for entities #162

Example Interaction

Expected Result

Mainstream

Opinionated