Upcasting

[prolic]

resolves #254

[coveralls]

Coverage decreased (-6.5%) to 93.031% when pulling 7151f0b on upcasting into 81bffea on develop.

[prolic]

For discussion:

• We need an instance of a message to create a new message - this means when updating events, you need to create a new class, f.e. UserRegistered => UserRegisterd + UserRegisteredV2 - is that okay?
• Should we maybe prefer to do upcasting with message name / payload / metadata only? If should remove the message factory from the event store, add our own (maybe GenericMessageFactory) - and use the real MessageFactory only within this plugin - how can this be achieved, if this is preferred?
• Do we need support for looking into the future of the stream per upcasting? Or can the upcaster itself store some information internally is required?

[coveralls]

Coverage decreased (-8.4%) to 91.107% when pulling 8782848 on upcasting into 81bffea on develop.

[mrook]

That was fast! I'll take an in-depth look as soon as I have time.

As for the discussion points:

• the new event class needs to exist before the upcaster can work.
• the question is whether you want each upcaster individually to handle deserializing the raw event (so that would indeed mean only passing metadata & payload), or present the deserialized domain event to the upcaster; I think I prefer the first version, as serialization may have changed over time as well.
• some upcasters could need this to merge events, they may be more interested in the past than the future however. In general I think this is something best handled in individual upcasters.

[prolic]

@codeliner @mrook tests now included

[coveralls]

Coverage increased (+0.02%) to 99.572% when pulling d4e89d6 on upcasting into 81bffea on develop.

[jkuchar]

Why not put upcast and downcast methods directly into events? These up/downcast methods are tightly coupled with event in particuar version anyway...

[prolic]

@jkuchar Single Responsibility Principle

[mrook]

@jkuchar because some upcasters would need information from other events (i.e., if you are merging) or emit multiple events (if you are splitting).

[prolic]

/cc @codeliner

[codeliner]

Can you take a second look on the message?

Why is the message turned into a messages array just to foreach over it?

You wanted to return messages, right? But $result is returned.
Foreach can be removed in either case.

[prolic]

You're right. I will fix this later.

[prolic]

Sorry, you are not right :)

At the first iteration of upcasters, you would have been right, but in the second iteration, I might have an array of messages, that may need further upcasting. see: UpcasterChainTest::it_chains_upcasts

This test would immediately break, if we would make the suggested change.

[codeliner]

ok, I see. Just a few lines of code but very tricky ;)

[prolic]

$this->innerIterator->next();

[prolic]

$this->innerIterator->valid()

[prolic]

There is still a bug in the iterator... I am trying to fix this now.

[prolic]

@codeliner I fixed the upcasting iterator

[coveralls]

Coverage increased (+0.03%) to 99.577% when pulling a578736 on upcasting into 81bffea on develop.

[prolic]

@mrook @codeliner your thoughts in the implementation?

[mrook]

@prolic looks fine for the initial implementation!

[codeliner]

LGTM

[mrook]

👍