[RFC] Add a form DTO maker for entities

[codedmonkey]

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

[cmartinezdnn]

Same issue happening to me. It looks like there is bug in new superset 4.0 version.

[sfirke]

Someone in Slack is seeing this too. I am running 4.0.0 in production and can successfully create a new alert, so this is not universally broken. My best guess is that there's something in the recipients validation checker that succeeds on my instance but fails for these people without telling them why. CC @fisjac as I believe they wrote some of this new validation.

[alexkyurtev]

please contact me if further information is needed , i`m also seeing this bug on my side

[fisjac]

Looking into this.

[padbk]

I am also seeing this issue, it is preventing our upgrade to 4.0.0 and is it not fixed in 4.0.1rc1

@michael-s-molina Could you help to get the above PR merged and included in 4.0.1 please? 🙏

[square-li]

+1 also having this issue with 4.0. Thanks!

[rusackas]

4.0.1 has been voted in... so this could become part of 4.0.2, coming soon. If you want to test/vote on these release candidates officially, please do join the [email protected] email list (let me know if you want instructions).

Normally we close issues with the PR that fixes them is merged to master. If that's the case here, we should close this, and the fix will be destined for the next release. Any objections?

[adimyth]

@rusackas I checked the release-announcements channel & other relevant channels on slack, but couldn't find a tentative timeline for 4.0.2 release

Any idea when would it be released?

PS - @fisjac thanks for fixing this 👍

[michael-s-molina]

Hi @adimyth. We generally try to release a patch every month. We just released 4.0.1 on Monday so 4.0.2 will probably be released mid June. @eschutho @sadpandajoe are working on 4.1 but I don't know if it will be released sooner.

[sfirke]

Alright folks watching this issue: Superset 4.0.2 is up for a vote! Anyone is welcome to test out the release and vote on it. Test it by deploying the docker image: apache/superset/4.0.2rc1 or apache/superset/4.0.2rc1-dev.

Vote by subscribing to the public email list https://lists.apache.org/[email protected] and replying to the email Michael sent this morning! Please test that this bug is fixed and that everything else works as it should. I am going to copy-paste this message onto a couple of other issues for visibility, sorry to folks who get multiple messages.

[giacomochiarella]

Hi everyone! 4.0.2 is under voting. I went through the changelog here and I cannot see any reference to this or this. Moreover this is still open. Anybody can give some clearance on this bug? I would down vote if this is not resolved actually (it is a regression, it was working in 4.0.0 and stopped in 4.0.1)

[michael-s-molina]

Fixed by https://github.com/apache/superset/pull/28409