neos / Neos.EventSourcing

A library for Event Sourcing and CQRS for Flow projects.
MIT License
44 stars 30 forks source link

Do we need a "when..." method inside the Aggregate #16

Closed sorenmalling closed 7 years ago

sorenmalling commented 7 years ago

Context for this use case: A Shop application where you create a catalog

Implementing CQRS in a project, I've come to a situation where the required when.. method leaves me with empty method.

class Catalog implements AggregateRootInterface {
    use EventSourcedAggregateRootTrait;

    /**
     * Catalog constructor.
     *
     * @param string $aggregateIdentifier
     */
    public function __construct(string $aggregateIdentifier)
    {
        $this->setAggregateIdentifier($aggregateIdentifier);
        $this->recordThat(new CatalogCreated());
    }

    public function whenCatalogCreated(CatalogCreated $event) {
    }

If i leave out the when method the apply method will throw a exception.

I'm well aware that I can override the apply method in AggregateRootTrait - but is this is the preferred way of managing this issue or should this method requirement be moved away from this part of the process.

@albe already gave me some answers on slack https://neos-project.slack.com/archives/guild-cqrs/p1473947411000401 regarding this but moved here for a further discussion about the implementation

albe commented 7 years ago

So basically the use-case is an Aggregate that emits Events which do not take part in its internal state. I guess we could lift the requirement and check if the method doesn't exist, then just skip (or log a warning in debug-level).

In the above case though, you should maybe add the identifier to the CatalogCreated event and set the aggregateIdentifier in whenCatalogCreated. Reason being that you will otherwise not be able to create any state for a specific Catalog instance, unless you couple your projections to the Event metadata implementation. Any information that you need in your domain or projections should be part of the Event payload.

sorenmalling commented 7 years ago

I think of "taking part in its internal state" as something on the same level as some other BC taking care of its state. Is that wrong? Isn't a event "fire and forget" and whatever part of a application that listens for that event can do something.

albe commented 7 years ago

Yes yes, absolutely right, but you should not depend on any information that is not directly part of the Event payload. So you are responsible for collection all relevant information when emitting your Event.

bwaidelich commented 7 years ago

I agree that the handler methods in the Aggregate should be optional (and I don't think we have to log anything if a handler is not implemented).

In the end an Aggregate is just a special EventListener – related to that I would even suggest to call the handler methods on*() like in other listeners

albe commented 7 years ago

I have to say I do prefer the when*() convention, because it fits better with proper event names. "OnUserWasRegistered" is not correct english, while "WhenUserWasRegistered" is. I would rather vote to change the other EventListeners to when* too.

robertlemke commented 7 years ago

I agree with @bwaidelich that the handler methods should be optional (but automatically be detected when they exist).

I also agree with @albe about naming them when*() and make sure that this pattern is consistently used in our code, including all other listeners.

bwaidelich commented 7 years ago

Great! I'm also perfectly happy with when*() as long as its consistent!

dfeyer commented 7 years ago

https://github.com/neos/Neos.Cqrs/pull/19 this change on -> when for the event listener

dfeyer commented 7 years ago

20 change the aggregate's event handler methods optional