Allow objects in message payload / enforce scalar values only for infrastructure reasons

[fritz-gerneth]

From Gitter earlier:

Sascha-Oliver Prolic
@fritz-gerneth you should be able to decide one day to integrate rabbitmq with a few simple steps, you shouldn't need to rewrite big parts of your application. Also you shouldn't know if the message is sent over the wire or not. Your stuff is supposed to just work, no matter what infrastructure is on the other side. Allowing objects in payload for most of the time (and sometimes not) only makes much bigger problems on the other end. I understand that you know your infrastructure and that you don't send messages somewhere else, but your limited situation is no reason for me to make the thing much more complex. Also how would you store events payload in the event store? This at least is a common problem with objects already, even if all your consumers would be PHP.

[...]
Fritz Gerneth
but I think the correct behavor would be that the default factories / converter throw the exception, not the message itself. that's what happening most of the time anway already. aka it should not the message to care about that but the infrastructure level. the default one does not allow objects, that's probably easier for everyone

Use-Case:

• Event contains a composable VO (tree like structure with many different implementations of a shared composable interface)
• Currently needs static mapping from array to object structure, very closed for extension, very open for modification, breaks modular approach and extensibility
• Handle built-in types better (e.g. date)

My stance on this:

• Messages itself should allow objects as payload
• Validation should only be done on the infrastructure level
  -- MessageConverter / MessageFactgory implementations are responsible for this
  -- For the event store, the event store strategy is responsible for this
• Default behvaior should still be to throw exceptions
• This allows custom MessageConverters / .. capable of handling objects in the payload

Alternatives:

• Re-implement command / event / query message types
• Fix inconsistent behavior that messages can be created with objects in payload but fail validation when metadata is updated (which triggers payload validation, payload validation should be checked on set)

[prolic]

@codeliner your thoughts on this?

[codeliner]

Moving assertions to message factory/converter sounds like a good idea, BUT:

The original idea was to make sure that messages can easily be sent over the wire and that they are immutable.
Later we removed payload from the message interface to allow the usage of custom serializers and such.

We wanted to make sure that people understand that a message is/should be a simple thing that you can serialize and push on a queue or store in a database. And once created, the message should not change.

But I do agree, that (depending on design decisions) messages can contain objects (hopefully value objects), too.

From a library POV it's a good idea.

From an educational POV it's not because people will start to use mutable messages and sooner or later run into issues with that.

So the question is do we want to educate people or do we want to provide a flexible library. Ideally both, but yeah the latter has higher priority for prooph components so if you want to provide a PR I'd accept it!
@prolic your thoughts on my thoughts? ;)

Fix inconsistent behavior that messages can be created with objects in payload but fail validation when metadata is updated (which triggers payload validation, payload validation should be checked on set)

This ^^ should definitely be fixed

[prolic]

@codeliner totally agree

[enumag]

Fix inconsistent behavior that messages can be created with objects in payload but fail validation when metadata is updated (which triggers payload validation, payload validation should be checked on set)

I believe this was fixed as a side-effect of #69. Not sure if it's enough to close this issue though.

[prolic]

@enumag: let's add a test-case, so we catch this once implementation details should change.

[fritz-gerneth]

Will check for further changes & next steps next week.

[prolic]

@fritz-gerneth any news on this?

[fritz-gerneth]

I finally got a chance to test this with a new context we're implementing. I added extraction / hydration calls to our message converter / factory. For converting messages to arrays this works as expected.

For restoring arrays there is a minor chance I had to apply: originally the factory I wrote only converted payload keys back to their object but left creation of the message to the original ::fromArray() call. Since the original implementation still checks for MessageDataAssertion the call now fails when re-creating the message. Overwriting this method solves this and from there on everything works as expected.

To the question of if the fromArray method should check or not.. I see this is part of the default message factory. If I have my own way of handling messages I have to use my own methods.. :) Eventually I'll replace the factories / converters with a full custom implementation anyway. So from my POV this can be closed.