[DTO] Add new make:dto command

[ckrack]

This command is added as a supplement to make:form.
To prevent problems with invalid state of entities when used in forms,
data transfer objects are a solution.
This command generates these from existing entities.
It can generate getters/setters and helper methods to fill and extract data from/to entities.

Closes #162

[ckrack]

From my side, this would be ready for merging.
I have also opened a PR to add this to the docs.

[weaverryan]

Hey @ckrack!

Awesome start - a lot of complex stuff here. But, doing cool things like this is part of what makes this bundle fun. I've added a first round of changes.

Cheers!

[ckrack]

I had a chat with @Pierstoval about his proposal for DTOs in EasyAdmin today and he raised the following issues:

doing $dto->fill($entity) isn't nice IMO because it can only use the public API of the entity itself
and this requires getters/setters
using DTOs is here to help us get rid of setters at least

To achieve this, he uses mutators on the entity ($entity->updateFromFoobarDto(FoobarDto $dto)) in which he can avoid using setters.

I agree to this and we should try to enforce best practices as much as possible - while making generated DTOs usable without modifications.

An approach could be:
When generating helper methods (currently $dto->fill() and $dto->extract()), use the constructor instead of extract() and add a mutator to the entity itself instead of generating a fill() method on the DTO.
This would mean adding code to the entity, while generating the DTO.

Simple example, given an Entity Foobar like this:

// src/Entity/Foobar.php
class Foobar {
    private $id;
    private $acme;

    // ... getters
}

Running the maker:

php bin/console make:dto Foobar Foobar
Add helper extract/fill methods? (yes/no) [yes]:
 > yes

 Generate getters/setters? (yes/no) [no]:
 > no

Would generate the following Dto:

class FoobarData {
    public $acme;

    public function __construct(?Foobar $foobar = null)
    {
        if (null !== $foobar) {
            $this->acme = $foobar->getAcme();
        }
    }
}

And modify the Entity with this

// src/Entity/Foobar.php

class Foobar {

    // ...

    public function updateFromFoobar(Foobar $foobar)
    {
        $this->acme = $foobar->acme;
    }
}

WDYT?

[Pierstoval]

I'm totally ok with this proposal 👍

Just as a side-note, this example:

class FoobarData {
    public $acme;

    public function __construct(?Foobar $foobar = null)
    {
        if (null !== $foobar) {
            $this->acme = $foobar->getAcme();
        }
    }
}

Should be replaced IMO with this:

class FoobarData {
    public $acme;

    public function __construct(Foobar $foobar = null)
    {
        if ($foobar) {
            $this->acme = $foobar->getAcme();
        }
    }
}

I'd even go further and only allow static constructors such as this:

class FoobarData {
    public $acme;

    public static function createEmpty(): self
    {
        return new self();
    }

    public static function fromFoobar(Foobar $foobar): self
    {
        $dto = new self();

        $dto->acme = $foobar->getAcme();

        return $dto;
    }
}

[mablae]

I'd even go further and only allow static constructors such as this

Yeah named static constructors like you suggest and an scalar conctructor like this:

public function __construct($acme)
{

        $this->acme = $acme;

}

Maybe Entity Class metadata could be used to derive the correct typehints, too

[Pierstoval]

Technically, a constructor can be called twice with $object->__construct();, while a static constructor will not affect a currently existing object, leading the update part only to proper mutators (or the Reflection API)

[karser]

Just my 2 cents: I usually add the most critical properties to the DTO's constructor. It reduces the number of lines, e.g, using
$dto = new FoobarData('value1', 'value2')
instead of

$dto = new FoobarData();
$dto->acme1 = 'value1';
$dto->acme2 = 'value2';

It is especial convenient in tests where a lot of DTOs instantiating is needed and they are not connected to the entities
So the @Pierstoval solution better fits to my approach.

class FoobarData {
    public $acme1;
    public $acme2;

    public function __construct(string $acme1 = null, string $acme2 = null)
    {
        $this->acme1 = $acme1;
        $this->acme2 = $acme2;
    }

    public static function fromFoobar(Foobar $foobar): self {}
}

[mablae]

@karser Why public properties?

[karser]

Indeed @mablae, getters/setters are more flexible. Probably, it'd be sensible for them to be configurable in make:dto - either use getters/setters or public properties.

class FoobarData {
    private $acme1;
    private $acme2;

    public function __construct(string $acme1 = null, string $acme2 = null)
    {
        $this->acme1 = $acme1;
        $this->acme2 = $acme2;
    }

    public static function fromFoobar(Foobar $foobar): self {}

    public function getAcme1(): ?string {
        return $this->acme1;
    }

    public function getAcme2(): ?string {
        return $this->acme2;
    }

    //setters if object is mutable
}

[Pierstoval]

Injecting values in the constructor is a nice idea @karser but I see here a big drawback for the Form component: it forces you to use a DataMapper.

The Form component highly uses getters and setters, and in the end our DTOs are just here to transport data from a layer to another, so having getters and setters is mostly here to guarantee the type & provide some autocompletion features.
Injecting values in the constructor is not natively supported by the Form component, nor is used by the Serializer (in case you want to create a DTO from in API e.g. from a JSON payload), so it's actually quite useless, unless you always do things manually (which is not supposed to be a common case when automating such process).

And by the way, with a proper IDE, creating a constructor for a property is done in a few clicks/keyboard shortcuts, as well as getters and setters, so not having a constructor by default does not sound very harmful to me.

[karser]

@Pierstoval It's only for some properties, not for all of them, because having more than 4-5 arguments in constructor makes it useless if you want to specify only last argument, e.g rather than new FoobarData(null, null, null, null, 'value5') I'd prefer $dto = new FoobarData(); $dto->acme5 = 'value5';.
So constructor with these properties can be created manually and as you suggested, adding method fromFoobar instead of constructor makes sense a lot.

btw Serializer supports denormalization from the constructor

[Pierstoval]

btw Serializer supports denormalization from the constructor

Didn't know that, thanks 👍!

For the rest, I still think public properties are kinda useless, especially because we don't have strict type yet (PHP 7.4 only, so not for now).

Do as you wish, after all people can do whatever they want with their DTOs, especially because the Form component can use them, but I don't think making this available as a direct feature is a good idea.
Newcomers and newbies might think that it's a potentially recommended feature, and this is something I often see when doing trainings to people that don't know Symfony yet.

The Maker sounds "magic" to newcomers, and they rely on the generated code as if it was a pure Symfony recommendation, and public properties has never been a really nice recommendation.

[weaverryan]

The blocker with this PR is that there's not just one way of doing a DTO. I see 3 main options - this is not a full list of possibilities, but trying to simplify into a few styles:?

1. Public properties (mutable)
2. Constructor args + getters, but not setters (immutable)
3. Getters + Setters (mutable)

(1) is so easy, and not something we should probably promote too widely (it's certainly fine in many cases - and devs that understand why vs when not will have no problem creating this very simple class).

For the other 2, I don't like having many ways to do things, but this seems like a special case to me. Could we add this question?

Does your DTO need to be mutable (usually "yes" if using the form component)?

• If no, option (2)
• If yes, option (3)

That would add 1 question that should create DTO's that would cover most situations.

Btw @ckrack this PR has been delayed by this complication and my delayed review. Are you still interested and available to work on it?

Cheers!

[Pierstoval]

The 3 solutions could be interesting, and solution 1 is almost identical to solution 3 with PHP 7.4 coming along, so we could instead propose the 3 ways of doing it, like:

Specify the type of DTO you want:
1. Mutable, with getters & setters (default)
2. Mutable, with public properties
        -> with PHP 7.4, we could introduce properties type-hints
           if DTO is based on an entity with Doctrine types or enough phpdoc
3. Immutable, with getters only
[Getters&setters by default] > 

[weaverryan]

That makes sense to me - allow the user to choose from these 3 different "styles" of DTO's. I always like less options, but these all seem like legitimate strategies, where the "best" is subjective and dependent on the person/situation.

Now, who wants to help finish this? :)

[Pierstoval]

@ckrack what do you need to finish this PR exactly? 😄

[odan]

@weaverryan What do you think about this option?

4. Immutable, with getter and wither (fluent setter)

[weaverryan]

Immutable, with getter and wither (fluent setter)

Hmm, maybe. It's really an extension to 3 (Immutable, with getters only) - it could be added now, later, or never :p. We should get at least a few options done initially - i.e. if there are other variants, those might be nice to add, but let's not make them blockers.

[ckrack]

Thanks for your input.
I'm trying to free some time to finish this.

[tacman]

Is it possible to have a fork of just the MakerBundle (https://github.com/symfony/maker-bundle/)? I'd be happy to test and give feedback, I need a DTO for a complex entity, would love to give approach this a spin. I have no opinion on where the files should go or how ->fill() should be called or what should be in the constructor. Whatever you implement, I'll try.

[maxhelias]

@tacman, you can use the fork of this PR and configure it on your composer.json like this : https://getcomposer.org/doc/05-repositories.md#vcs

[conradkirschner]

Is this PR still in Review? What needs to be done?

[weaverryan]

This PR needs a champion to carry it to the finish. @ckrack has done a great job getting it started, but he may or may not (he can correct me) have as much time as he'd like to finish it :). Anyone else can take this work and continue on a new PR if you have some time and willingness!

[ckrack]

This should be ready for another review @weaverryan.

I've added the three style options.

 Specify the type of DTO you want::
  [1] Mutable, with getters & setters (default)
  [2] Mutable, with public properties
  [3] Immutable, with getters only
>

When using 1 and 2, you will get a constructor that accepts the Entity and uses the getters, setters or public properties to setup the Dto with values.

When using the immutable style, you'll get a constructor with all properties (ordered by mandatory -> optional with defaults -> optional) and a named constructor fromFooEntity which accepts the FooEntity in this case.

The mutator was moved to the Entity

Add mutator method to Entity (to set data from the DTO)? (yes/no) [yes]:
 > 

This will create FooEntity->updateFromFooData(FooData $fooData).

[duboiss]

Little up/ping for this 2 years old PR just waiting to be reviewed @weaverryan 😄

[seb-jean]

PR interesting :)

[excitedbox]

Is there anything happening with this? Seems pretty cool, but seems to have gotten stuck right before the finish line.