webdevilopers / php-ddd

PHP Symfony Doctrine Domain-driven Design
200 stars 11 forks source link

Primitives instead of value objects in Domain Events #37

Open webdevilopers opened 4 years ago

webdevilopers commented 4 years ago

Came from:

Recently I started a symfony project w/ @prooph components. Looking at the bluebrints I recognized the creation of events has changed.

 class User extends AggregateRoot
    {
        /**
         * @var Uuid
         */
        private $uuid;
        /**
         * @var string
         */
        private $name;
        /**
         * ARs should be created via static factory methods
         */
        public static function nameNew(string $username): User
        {
            //Perform assertions before raising a event
            Assertion::notEmpty($username);
            $uuid = Uuid::uuid4();
            //AggregateRoot::__construct is defined as protected so it can be called in a static factory of
            //an extending class
            $instance = new self();
            //Use AggregateRoot::recordThat method to apply a new Event
            $instance->recordThat(UserWasCreated::occur($uuid->toString(), ['name' => $username]));
            return $instance;
        }

        /**
         * Every AR needs a hidden method that returns the identifier of the AR as a string
         */
        protected function aggregateId(): string
        {
            return $this->uuid->toString();
        }
        protected function apply(AggregateChanged $event): void
        {
            switch (\get_class($event)) {
                case UserWasCreated::class:
                    //Simply assign the event payload to the appropriate properties
                    $this->uuid = Uuid::fromString($event->aggregateId());
                    $this->name = $event->username();
                    break;
                case UserWasRenamed::class:
                    $this->name = $event->newName();
                    break;
            }
        }
    }
    /**
     * ProophEventSourcing domain events are of the type AggregateChanged
     */
    class UserWasCreated extends AggregateChanged
    {
        public function username(): string
        {
            return $this->payload['name'];
        }
    }

Before there was a lot of boilerplate going on e.g. from an earlier project:

final class DormerElementCreated extends AggregateChanged
{
    /** @var DormerElementId */
    private $dormerElementId;

    /** @var VariableName */
    private $variableName;

    /** @var string */
    private $variableDescription;

    /** @var DormerPart */
    private $dormerPart;

    /** @var MeasurementUnit */
    private $measurementUnit;

    /** @var float */
    private $workingHours;

    /** @var int */
    private $surcharge;

    public static function with(
        DormerElementId $dormerElementId, VariableName $variableName, string $variableDescription,
        DormerPart $part, MeasurementUnit $unit, float $workingHours, int $surcharge
    ): DormerElementCreated
    {
        $event = self::occur(
            $dormerElementId->toString(),
            [
                'id' => $dormerElementId->toString(),
                'variableName' => $variableName->toString(),
                'variableDescription' => $variableDescription,
                'dormerPart' => $part->toString(),
                'measurementUnit' => $unit->toString(),
                'workingHours' => $workingHours,
                'surcharge' => $surcharge,
            ]
        );
        $event->dormerElementId = $dormerElementId;
        $event->variableName = $variableName;
        $event->variableDescription = $variableDescription;
        $event->dormerPart = $part;
        $event->measurementUnit = $unit;
        $event->workingHours = $workingHours;
        $event->surcharge = $surcharge;

        return $event;
    }

    public function dormerElementId(): DormerElementId
    {
        if (null === $this->dormerElementId) {
            $this->dormerElementId = DormerElementId::fromString($this->aggregateId());
        }

        return $this->dormerElementId;
    }

    public function variableName(): VariableName
    {
        if (null === $this->variableName) {
            $this->variableName = VariableName::fromString($this->payload['variableName']);
        }

        return $this->variableName;
    }
}

Most of the getters were creating value objects from the payload. This was really annoying when dealing with arrays. Most of the time you had to implement toArray and fromArray methods and loop them through array_map methods.

This change made me think. Then I read this post by @heynickc:

And followed some other discussions:

@prooph for instance keeps the symfony structure for convenience too:

I'm really beginning to like this approach. What do you guys think?

See also:

webdevilopers commented 4 years ago

@yvoyer suggests on Twitter:

Primitives in Event's attributes, Value object as returned value of methods.

In the @prooph example methods return the "primitive" payload:

    class UserWasCreated extends AggregateChanged
    {
        public function username(): string
        {
            return $this->payload['name'];
        }
    }

Then the value objects are created inside the aggregate:

       protected function apply(AggregateChanged $event): void
        {
            switch (\get_class($event)) {
                case UserWasCreated::class:
                    //Simply assign the event payload to the appropriate properties
                    $this->uuid = Uuid::fromString($event->aggregateId());
                    $this->name = $event->username();
                    break;
                case UserWasRenamed::class:
                    $this->name = $event->newName();
                    break;
            }
        }

@gquemener @codeliner @prolic Any reason why you havn't skipped this step entirely using payload directly then?

       protected function apply(AggregateChanged $event): void
        {
            switch (\get_class($event)) {
                case UserWasCreated::class:
                    //Simply assign the event payload to the appropriate properties
                    $this->uuid = Uuid::fromString($event->aggregateId());
                    $this->name = $event->payload()['username'];
                    break;
            }
        }
gquemener commented 4 years ago

I guess it's up to everyone's preference.

Having only primitive values within the messages (commands, queries, events) help serializing them. However, I believe that getting value objects from them or primitive (and creating value object within the entity) is valid in both case.

Personally, I tend to vote in favor of always storing internally primitive value within an entity or a message and exposing them through value object (created on the fly), but that solely my current opinion.

Finally, I don't like exposing the whole payload of the message, it kind of look like breaking encapsulation (makes me think of a demeter's law break). Imagine UserWasCreated::$payload was an instance of Payload. It means that you would need to call $event->payload()->username(), which is smelly IMO.

But once again, it mostly depends on other factors like the size of your project, the number of people working on it and the coding standard you comply with, and what you show could totally be ok.

Here's an example of event, encapsulating value object creation: https://github.com/gquemener/PhpMyOssContrib/blob/master/src/Contribution/Domain/Event/ContributionOpened.php

webdevilopers commented 4 years ago

I agree and like your example. Currently the prooph example looks very "aenemic":

    class UserWasCreated extends AggregateChanged
    {
        public function username(): string
        {
            return $this->payload['name'];
        }
    }

Not even properties need to be defined - it all "depends" on the payload.

prolic commented 4 years ago

@webdevilopers yes, quick and dirty prototyping to help get users up to speed. it's not a recommended best practices section, only something to demonstrate how it works.

webdevilopers commented 4 years ago

I get it @prolic . In the end I would keep the primitives inside the event for serializing and skipping the toArray boilerplate on the value objects. But when the events are finalized I use methods that create the value objects from the payload which of course can be arrays.

Would you add a Wither with typed properties to the event similar to the attempt by @gquemener then instead of calling "occur" directly?

        public static function nameNew(string $username): User
        {
            //Perform assertions before raising a event
            Assertion::notEmpty($username);
            $uuid = Uuid::uuid4();
            //AggregateRoot::__construct is defined as protected so it can be called in a static factory of
            //an extending class
            $instance = new self();
            //Use AggregateRoot::recordThat method to apply a new Event
            $instance->recordThat(UserWasCreated::occur($uuid->toString(), ['name' => $username]));
            return $instance;
        }

Could the "occur" method not be made protected / private then in order to ensure the usage of a Wither?

Example:

final class UserRegistered
{
    public static function with(UserId $id, string $username):
    {
        $instance = new self();
        $instance->occur($id->toString(), ['username' => $username]);

        return $instance;
    }
}
prolic commented 4 years ago

If you have a need for it. I usually don't. Sascha-Oliver Prolic

Am Di., 10. Dez. 2019 um 15:23 Uhr schrieb webDEVILopers < notifications@github.com>:

I get it @prolic https://github.com/prolic . In the end I would keep the primitives inside the event for serializing and skipping the toArray boilerplate on the value objects. But when the events are finalized I use methods that create the value objects from the payload which of course can be arrays.

Would you add a Wither with typed properties to the event similar to the attempt by @gquemener https://github.com/gquemener then instead of calling "occur" directly?

    public static function nameNew(string $username): User        {            //Perform assertions before raising a event            Assertion::notEmpty($username);            $uuid = Uuid::uuid4();            //AggregateRoot::__construct is defined as protected so it can be called in a static factory of            //an extending class            $instance = new self();            //Use AggregateRoot::recordThat method to apply a new Event            $instance->recordThat(UserWasCreated::occur($uuid->toString(), ['name' => $username]));            return $instance;        }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/webdevilopers/php-ddd/issues/37?email_source=notifications&email_token=AADAJPCGQXYFTV45MTZLXCTQX7NBRA5CNFSM4JY5RTAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGQH6VI#issuecomment-564166485, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADAJPDQWEOHJVXZFAWJWM3QX7NBRANCNFSM4JY5RTAA .

codeliner commented 4 years ago

IMHO you have to distinguish between the event itself (the fact stored in the event store) and the representation of the fact in different contexts. F.e. we usually work with a mapping, so our events follow a naming convention: [Context].[EventName] -> ParcelTracking.ParcelDelivered

In the owning context we have a dedicated ImmutableRecord class representing the event. The record is strongly typed. No primitives are exposed. Every information is represented by a type class. Most of the time this is a value object from the domain model, but it could also be a type defined under the event namespace.

If the same event is consumed by another context, we usually put the information in a "GenericEvent" class. You can compare it with a PSR-7 request. It is an immutable class that gives you access to the information of the event.

As you can see it depends on where the event is used, but one thing is always true: the Information carried by the event never changes (unless we migrate events on database level).

That said, I would not pass primitives around in my domain model. When information leaves the event (calling a getter of the event class), the information should be represented by a type. If the type is a value object and the VO changes for whatever reason, then you can keep an old version of the VO as a type under the event namespace. No need to version the VO:

Domain\Model\OldValueObject -> move to -> Domain\Model\SomethingHappened\OldValueObject -> add new -> DomainModel\ModifiedValueObject

Even better: most of the time you can use the event getter to upcast a primitive before passing it to a modified value object. This way, the rest of the domain model does not need to care about the version of the event.

Disclaimer: Maybe we need to differentiate between Types and Value Objects. In PHP they often look the same. For me a Type represents information. A Value Object on the other hand can protect invariantes and contain domain logic. If both can be handled within the same class - fine. But I still have the option to separate the Type from changed domain logic. No need to fallback to primitives and a higher risk of bugs in the software.

webdevilopers commented 4 years ago

Thanks for your explanation and the examples @codeliner ! I agree.

I guess I was a little bit confused by the RAD demo code. But as Sascha mentioned:

@webdevilopers yes, quick and dirty prototyping to help get users up to speed. it's not a recommended best practices section, only something to demonstrate how it works.

Maybe this code could be improved for future users.