prooph/pdo-event-store

Projection implementation dependent on Message interface

camilledejoye opened this issue · 4 comments

Hi,

First of all, thanks for the work done!

To gave a bit of a context: I'm trying to use Prooph, with the PDO implementation and Symfony bundle, to work with my model without changing it.
I don't use Prooph event sourcing component at all.

Since my domain events doesn't implement the Message interface I created a wrapper for them.
So before storing I wrap them, and with a listener on "load" I unwrap them.

So far so good, until... I ran a projection!

That's the guilty part:

if (! isset($this->handlers[$event->messageName()])) {

Which bring me to ask myself, why coupled the projection with the Message interface ?
At least for this part, didn't check all the projection system yet 😄

My point is that projections and messaging are two different and independent parts.
To go along with this idea, the return type provided by the EventStore interface for the load/loadReverse methods is Iterator.
So there is no guaranty, no matter what, that we will be provided with a object implementing the Message interface.

And just a little bit further:

$result = $handler($this->state, $event);

We provide the event, just as it is to the handler.
So it feels pretty strange to ask for the type of the event through a method instead of using \get_class($event) directly.
Because it would mean that we expects to have a situation like that:

$projector->fromStream($streamName)
    ->when([
        AType::class => function ($state, AnotherType $event) {
            // Handle the AnotherType event when AType is given
        },
    ]);

Given all that, is there a reason not to start a PR race? 😋

This would be a BC break and cannot be done. EventStore v8 is already in the pipeline though.

So I guess that mean I didn't misunderstand the situation at least.
And I was more thinking about something like:

$messageName = $event instanceof Message ? $event->messageName() : \get_class($event);

To avoid to break anything indeed.
But I will keep an eye on the dev branch for now and use composer VCS repository for my project :)

@elythyr if you can provide a test-case and a patch, that seems to be reasonable to me.

It doesn't worth the time.
I didn't saw any unit test allowing to mock an event store and control the load method result.
To reproduce what is done in the existing tests it would require to create a new event store that handle domain event which does not extend the Message interface.
A listener to change the results of the PdoStreamIterator (because its only able to create objects extending Message).
It would also require a new Aggregate, with new events, etc.

And I'm not sure what to test anyway, that we provide an object with it's own type and not another one ?

Since you are already working on the next release, I don't really see the point to go through all that :)