fromArray and toArray don't need any replacement in my opinion - MessageConverter and MessageFactory have that covered.

payload and setPayload should go into separate interface named PayloadMessage or MessageWithPayload. Default implementations of MessageConverter and MessageFactory would trow an exception for messages not implementing PayloadMessage - the user needs to implement their own array conversion logic for no-payload messages.

@enumag How can a default message converter and factory look like, if there is no payload method and no toArray/fromArray can get called? I don't want to force everyone to implement this by themselves.

This comment has been minimized.

Something like this I think. Those who are using payload would not be forced to implement anything. Just their messages would need to implement the new PayloadMessage interface but they already have those methods implemented.

This comment has been minimized.

edited

I have a different idea that should work for me and is a less of a BC break:

separate the properties and some of the methods (eg the withMetadata) from DomainMessage to a new trait

move payload() from Message to DomainMessage

I still think that DomainMessaage fromArray/toArray should be deprecated in favor of MessageConverter and MessageFactory but with this idea it's pretty much a separate issue which can be considered independently.

This comment has been minimized.

But I do agree that it should be the task of MessageConverter and MessageFactory to convert from/to array including message payload.

The thing is, without payload the message interface feels incomplete. Again, it's like removing ServerRequestInterface::getBody() from PSR-7
so we decided to keep payload in the interface and encourage people to get used to it ;)

This approach works really really well. And if you ask me everybody should use it. But again, your request is valid and one can ask why MessageConverter and MessageFactory exist.

The simple answer is: Because the two work with the entire message object and covert it. Payload is just one property and the return type of its getter method is an array.

The conflict is, that there is no way to inject a custom payload serializer into a message other than invoking a static method of the serializer in Message::payload(). And that's definitely against OOP best practice so I cannot suggest doing this instead. 🤔

move payload() from Message to DomainMessage

Not my favorite. We use Message interface with custom implementations in some projects. So we would recognize the missing payload method.

I'm really not sure if we should do that. On the one hand it would make the concept of MessageConverter/Factory more complete but on the other hand it could cause a lot of trouble in existing projects relying on that interface.

This comment has been minimized.

edited

It became a common approach to create value objects insight message getter methods like shown in prooph-do
This approach works really really well. And if you ask me everybody should use it.

I have to disagree here. We tried that at first but to say it didn't work very well would be an understatement.

The first problem is the assertions in setPayload. Since some commands are directly connected to the API we can't use these assertions because we would only get the first error instead of all of them. I really don't want to duplicate the validation somewhere else. Prooph-based application needs a lot of writing already so I need to simplify as many things as possible.

The second problem is that the getter methods are a real pain to write. With normal objects, IDE can just generate the getters for you but it can't generate these getters. When some form (and by extension the command connected to it) has a few dozen fields, writing all the getters gets really painful really fast.

Third problem is null values handling. It's very easy to forget that some value is nullable when writing the getters. The getter would need to look like for example return isset($this->payload['until_date']) ? new DateTimeImmutable($this->payload['until_date']) : null;. If you forget the ternary then in the better case you get a fatal error in production and in the worse case the problem is silently ignored giving you some random data instead (such as current date instead of null). After fixing about 10 problems like that every week I was forced to admit that the payload approach simply doesn't work.

So yeah, I had a very bad experience with payload. I'd like to hear how you approach it to get around these issues but even if you somehow have an ultimate solution I'm very unlikely to switch back to payloads. Right now payloads feel like a newbie trap to me and I could not recommend it even to my enemy. These problems should be addressed in the docs in my opinion.

Anyway all of that is a bit off topic. I understand your viewpoint so I'd like to suggest a compromise - remove DomainMessage::fromArray/toArray in favor of Converter/Factory, but keep payload as is. The default implementations would use the payload of course but custom implementations might not. At the same time payload and setPayload methods should not be used anywhere in prooph libraries other than the default Converter/Factory.

This comment has been minimized.

thx for the detailed pain report :D It's interesting. I had very bad experiences with serializers in the past and therefor try to avoid them. Much simpler to map from/to value objects directly. But ok, it does not work for you.

The compromise is not bad. Let's see if we can do that. And I'll also think about payload again ...

This comment has been minimized.

I aggree with @enumag on this topic for the same reasons. In my opinion the current array-notion of a message is a storage detail (or bus detail) which a message should not be aware of itself. For us this is mostly tedious copy and paste with some quick unit tests to make sure all possible combinations work (again mostly copy and paste). Both tasks are error-prone though.

Truethfully I am not a big fan of keeping the payload method in the interface either. For any custom implementation this method would not be implementable, leaving a non-implemented / working method on the class (e.g. any automatic object serializer would not be able to be called from within this method). This maybe should go into the PayloadConstructable interface instead?

This comment has been minimized.

edited

@prolic Works for me but I'd like to also have a class/trait with the non-payload related properties and methods from DomainMessage (such as metadata). I can implement it on my side of course if you don't want it, I just thought it might help other people too.

This comment has been minimized.

edited

@codeliner By the way as for serializers I do have to admit that our Converter/Factory are using some PHP reflection magic which is not very pretty. However dispite the code being a bit of a black magic, we never had any problem with these classes.

This comment has been minimized.

I was thinking about this today again and suddenly I had two insights. It's not yet worked out really, however I want to share it here, maybe you guys have some thoughts about it as well.

I looked at the message interface again and thought about what is really necessary. And actually nothing is. For example the service bus can work with everything, it does not have to be an instance of message. So why not split up the interface completely? MessageWithName, MessageWithMetadata, UniqueMessage (uuid), PayloadMessage, MessageWithTimestamp, ...

This would allow for maximum flexibility, plugins can do type checks for specific features they are interested in (or none at all). Again, just an idea, not really thought through.

I was thinking about the payload itself and the message converter again. The converter gives an array from an object. Ok, but how can an array be sent over wire? Not really. That's why we json encode it afterwards (see every single code that handles that, f.e. message producers or event store implementation). We are never really interested in an array, but in a string (or bytes to be more precise). The string could also be xml or some bytes of google protobuf. So actually the converter should return bytes and the factory create an object from bytes. This way we push json encode, xml creation, protobuf generation, whatever to the message converter. Nice thing about google protobuf support: very compact message for transfer over wire (if you need an example, check event store client).

This comment has been minimized.

On 1: Interface composition only:
Not sure if this might be overkill. But I certainly like the idea. Different parts of an application sre only interested in certain aspects on an object. Creating dedicated interfaces is also a good way to hide other aspects of an object.
Interfaces I would see here are

MessageName

MessageMetadata

UniqueMessage

MessagePayload

MessageDate

MessageType (?, should maybe only be an interface instead, see below)

There is a problem with this granular approach though: you cannot type-hint to a mixture of those interfaces without introducing helper-interfaces or checking for multiple interfaces within the method. Former looses type-hinting and is tedious to write, first defeats the purpose of this separation in the first place and might not always be possible / desireable.

Also, I would suggest to remove the DomainMessage from the inheritance hierachy for most parts. There are simply some Commands / Events / Queriers that are not part of my domain but only of technical nature (is the DomainEvent actually required?):

This comment has been minimized.

This is not true for what we are doing here. The new prooph/common lib will be used by the new major release of prooph/event-sourcing (and prooph/micro). prooph/event-store v7 is not affected at all. prooph/event-store v8 is in the pipeline and will be a 100% rewrite.

In case you're wondering:

prooph/event-store v8 will be a server implementation and communication will be done via prooph/event-store-client

which would bring up the next question: would be need those Command / Query / Event interfaces at all? As you already suggested those are only relevant for the de/serializing messages. Take the v8 event store as an example: does it really care how the EventData is created? Should it not be the job of a message converter/factory to then create any event and cast it to an EventData object (and vice versa)? As long as the message converter can convert the event, the store does not care at all about whichever interfaces the event is implementing, right? That would suggest to remove any interface requirements and push the responsibility completely to the message converters. Of course a default implementation of Events and converts still might provide them for ease of use.

This would of course lead down a trecherous path where anything can be an event / command / query (as long as a converter can convert it). The bus probably would at least need the MessageName to dispatch it accordingly.

Part of this would suggest to merge & rename the message converter / factory to a single EventDataHydrator or alike (e.g. to Any input <-> EventData).

This comment has been minimized.

This comment has been minimized.

I looked at the message interface again and thought about what is really necessary. And actually nothing is. For example the service bus can work with everything, it does not have to be an instance of message. So why not split up the interface completely? MessageWithName, MessageWithMetadata, UniqueMessage (uuid), PayloadMessage, MessageWithTimestamp, ...

Could really be one interface aggregating the others. I don't think there is a strong advantage here, unless there is a practical use-case for messages that implement one interface but explicitly NOT an implementation of another.

I was thinking about the payload itself and the message converter again. The converter gives an array from an object. Ok, but how can an array be sent over wire? Not really. That's why we json encode it afterwards (see every single code that handles that, f.e. message producers or event store implementation). We are never really interested in an array, but in a string (or bytes to be more precise). The string could also be xml or some bytes of google protobuf. So actually the converter should return bytes and the factory create an object from bytes. This way we push json encode, xml creation, protobuf generation, whatever to the message converter. Nice thing about google protobuf support: very compact message for transfer over wire (if you need an example, check event store client).

My tip is getting rid of all this manual stuff: be extremely aggressive with reflection and just estract the heck of objects, recursively. Making people work with strings is possibly even worse, IMO: less control to be given to consumers here with out-of-the-box defaults. Yes, if people want protobufs, add an interface to do something like class MyMessage implements YesIKnowWhatIAmDoingPleaseLetMeSerializeStuffOnMyOwn.

This comment has been minimized.

They are currently part of any command, event and query I have build in my projects. But to be honest, I hate this inheritance from any third party abstract class like "DomainMessage" or any interface implementation.

Commands are mostly independent when no message broker is in use. Also queries don't need any interface at all.

An event has to be storable. So all what we need is something like an EventSerializer or a chain of serializer, which knows how to serialize an event object.

By the way.. Many prooph components require implementation of the Message interfaces without testing the type of the objects at all.

This comment has been minimized.

edited

An event has to be storable.

Not every event has to be storable.

But ultimatively this is what @prolic suggested. Only certain use cases require some interfaces (or would make it easier). But the entire hierachy, up to DomainMessage would only provide a easy to use default implementation.

This comment has been minimized.

edited

Not every event has to be storable.
Of course. I thought of events for 'stored state'. But there are also events which are not related to this issue.

So what we need is an interface for something like a "MessageReader", which could provide necessary values required from some prooph components for a message.

I would also like it to be more strict in prooph components. So if any method requires a Message as argument, than it should get one. e.g the EventStore should always get an DomainEvent object. If a user would implement such a component, but has only messages without any implementation detail of current interfaces / abstract classes, than he has to use a MessageConverter.

This comment has been minimized.

@prolic To be honest I don't see much value in spliting the interface completely. It would complicate a lot of code where you could no longer use a Message typehint but would have to use instanceof multiple times to throw an exception if one of the interfaces needed in that context is missing. The name, date, type and uuid are a necessity for every message. Or do you have a use case where it would be good to NOT implement one of these interfaces? The only things that could be separated are the metadata and payload in my opinion.

I'm not sure about your idea about converting message <-> json instead of message <-> array <-> json. Even if the converter would return string, it would internally still use the array and most likely depend on two other classes to do the message <-> array and array <-> json conversions. Hiding the array from view sounds good to me at first glance but I wonder if it won't cause problems later on. One thing that comes to mind is that when you change an event class and need to update all such events in event store accordingly then it might be better to work with arrays for the migration.

Next can you explain that EventData class from prooph v8? I'm completely confused about the isJson property.

Looks like @Ocramius is suggesting exactly what I'm doing - serialization using reflection and @var annotations. Personally I'd refrain from going that way in prooph until PHP has typed properties (@var annotations are a pain to deal with) but I if you want to go that way, I can help for sure.

This comment has been minimized.

@enumag In EventData the isJson property tell, that the data is a json encoded string. So whenever isJson is set to true, you can expect data to be in json format, if it's false, data can be a simple string, xml, protobuf, whatever