search

neos / Neos.EventSourcing

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

Reconsider usage of traits #17

Closed bwaidelich closed 8 years ago

bwaidelich commented 8 years ago

TL;DR;

This one is not related to any highlevel CQRS/ES topic but the current implementation uses traits and I want to suggest to replace them by abstract classes instead.

Background

We currently use nested traits for AggregateRoot and EventSourcedAggregateRoot. I'm very skeptical towards the general usage of traits in user-facing code (see Marco's talk for some valid counter-arguments).

Currently every aggregate needs to have

class SomeAggregate implements EventSourcedAggregateRootInterface
{
    use EventSourcedAggregateRootTrait;

And I fail to see why this should be any better than

class SomeAggregate implements AbstractEventSourcedAggregate
{

(the AbstractEventSourcedAggregate implementing EventSourcedAggregateRootInterface obviously to make that replaceable).

Note: If we could get away without inheritance and use composition that would be even nicer IMO but I don't see a pragmatic way with aggregate roots.

robertlemke commented 8 years ago

Fully agree on this, let's not use traits!

albe commented 8 years ago

Well, the idea of traits is to have composable behaviour. I think the issue Marco brought up does not quite fit our case (we don't try to copy code, but provide default behaviour - so more towards his last example of a valid case of providing collection behaviour for multiple classes). Edit Note: Maybe if we renamed the trait use DefaultEventSourcedAggregateRootBehaviour; it would be more expressive.

But I also see, that in our case the Trait does not really offer any benefit over just providing an abstract class. So since abstract classes/inheritance are better known I'd also vote for going that way.

dfeyer commented 8 years ago

No objection from my side ... I start with trait to not force inheritance in the developper domain, but maybe aggregate it's not needed ? As PHP don't support polymorphin, if we provide only an abstract class, and the application need an aggregate that inherit from an other class, we interfere with the domain I think (devil advocate more here)

robertlemke commented 8 years ago

@dfeyer I think in practice you can usually get around that problem (having to extend multiple classes), in a worst case with interfaces + traits.

Well, it's not 100% pro or con, but I still tend to remove traits, would feel better with it.

albe commented 8 years ago

So I guess we all agree that we can just get rid of the AggregateRootTrait.