Use MessageConverter in persistence strategies

[enumag]

I wanted to change the behavior how a message is serialized (I'm not using Message::payload()). In order to do so I was forced to implement a custom persistence strategy.

In my opinion it's not a responsibility of a PersistenceStrategy to do the serialization. And it's not a responsibility of a Message either in my setup.

Recently I found a MessageConverter interface in Prooph/Common and I believe the responsibility belongs there and so it should be used here in the persistence strategies. Then I could simply change the converter implementation.

Note: I would like to remove the payload method from Message interface later in another PR and move it to DomainMessage or a new MessageWithPayload interface.

[prolic]

Seems like a BC to me.

[enumag]

Well obviously. There is no way to do this without a BC break.

[coveralls]

Coverage decreased (-0.4%) to 89.747% when pulling 1e8459e on enumag:message-converter into 54e3e44 on prooph:master.

[prolic]

ping @codeliner

[enumag]

Technically I could make the argument for persistence strategies nullable and create a fallback implementation if null. That way it would be fully backwards compatible. Should I implement it that way? Possibly with a deprecation if the argument is not provided?

EDIT: Done.

[enumag]

ping @prolic

[prolic]

ping @codeliner

[prolic]

Why not use NoOpMessageConverter directly? What is this class for? Why is it deprecated?

[enumag]

It's deprecated because such class does not belong to this package and is only here for BC.

NoOpMessageConverter should be located in prooph/common. It is already there but requires DomainMessage which is a slightly different case.

I can add the converter to prooph/common, use it here and remove this class - of course it will require increase in required prooph/common version which technically means losing BC with older prooph/common. Are you fine with that? If so, any tip for the class name since NoOpMessageConverter is already taken?

Or I can use the existing NoOpMessageConverter from prooph/common which is also a BC break because it requires DomainMessage.

Basically it depends on how far you want to go with the backwards compatibility. Currently this PR has full BC as requested.

[prolic]

Maybe just call it default message converter. About deprecated: this means it will be removed some day. This wouldn't be the case for any future minor release, and next major release will be 100% different from what we have today. So I'd say remove thw deprecation notice.

…

On Wed, May 23, 2018, 20:07 Jáchym Toušek ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/CompatibilityMessageConverter.php <#153 (comment)> : > + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Prooph\EventStore\Pdo; + +use Prooph\Common\Messaging\Message; +use Prooph\Common\Messaging\MessageConverter; + +/** + * @deprecated This class is used for backwards compatibility. It's recommended to use a different MessageConverter implementation such as \Prooph\Common\Messaging\NoOpMessageConverter. + */ +class CompatibilityMessageConverter implements MessageConverter It's deprecated because such class does not belong to this package and is only here for BC. NoOpMessageConverter should be located in prooph/common. It is already there but requires DomainMessage which is a slightly different case. I can add the converter to prooph/common, use it here and remove this class - of course it will require increase in required prooph/common version which technically means losing BC. Are you fine with that? If so, any tip for the class name since NoOpMessageConverter is already taken? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvNtQvQc-1tHWQDoYHPOe5OFM1EB0ks5t1VEbgaJpZM4T5_Ga> .

[enumag]

@prolic Done. There is still the issue that such converter doesn't really belong here. I think I'll add a copy to prooph/common later.

[prolic]

ping @codeliner, your thoughts?

[codeliner]

LGTM

[prolic]

thanks @enumag

next release will be probably this weekend, still waiting for another PR to be finished

[enumag]

@prolic That's fine. I'm not in a hurry to use this anyway. Thank you!