Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Message serializations #28

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

makasim
Copy link
Member

@makasim makasim commented May 15, 2019

ref #27

@@ -1 +1,2 @@
/vendor/*
/.idea/
Copy link

@athlan athlan May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be defined in user's global gitignore. Each user uses own tools.

@makasim
Copy link
Member Author

makasim commented May 22, 2019

I've been thinking a lot about this issue and I dont see how this could be solved without introducing serializer interface for a message body.

@Sevavietl what do you think?

@Sevavietl
Copy link

@makasim I do not know if it worth to be so strict on message body itself. Maybe we can loosen it a bit like it is done in this PR, but control it with the serializer in context, for example. Like it is done in rdkafka enqueue implementation. Do you see any caveats in such approach?

I will look deeply into the JMS, maybe find there some direction. It is hard to get the answer from the top of my head. Let's come back to this later.

@makasim
Copy link
Member Author

makasim commented May 22, 2019

Maybe we can loosen it a bit like it is done in this PR, but control it with the serializer in context, for example. Like it is done in rdkafka enqueue implementation. Do you see any caveats in such approach?

Than a serializer is bound to transport, and if you switch from one to another the code will break.

@Steveb-p
Copy link

Than a serializer is bound to transport, and if you switch from one to another the code will break.

I'd say it's user's responsibility to select and attach correct serializer. If one want to shoot himself in the foot by using different serialization method for producing and consuming, then it's his fault for doing so.

@Sevavietl
Copy link

Sevavietl commented May 29, 2019

@makasim, @Steveb-p, I am also against making serializer bound to specific transport. It is better to resolve it from configurations: di container is a must-have nowadays.

So I see it as transport, message, and serializer that is configured for the specific message/transport combo. This way there is no coupling between the serializer and message, serializer and transport. And as a plus, the same serializer can be used across the project, without the need for adapters and wrappers. Well, there is one adapter to make serializer to satisfy enqueue internal serializer, as there is no psr for the serializer.

{
return $this->body;
}
}
Copy link

@Steveb-p Steveb-p May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please insert newline and make git happy :)

EDIT: The same goes for the test file for it.

@Steveb-p
Copy link

Steveb-p commented May 29, 2019

In my opinion, since this is an interface only, there should be nothing about how Producer handles Messages. How serializer is attached (or if it event relies on it) should be outside the scope of queue-interop.

Just the fact that a Message object is passed should be sufficient.

Attaching Serializer to anything should be an implementation detail.

As I see it:
Context::createMessage should accept anything as $body argument.
EDIT: As a matter of fact, I would make 1st argument mandatory. What's the point of not passing anything as body? What would be the use-case?
Producer::send should accept any Message. How it handles serialization or if it needs one is up to implementation. It could as well be that Message is an instance of JsonSerializable or implement __sleep/__wakeup and be passed to built-in PHP serializer, we do not really care.
The same as above applies to Consumer. It's programmers responsibility to unserialize.

What's the purpose of the trait that is added in PR?

@Steveb-p
Copy link

What is the state of this PR? @Sevavietl will you continue on it, or you don't have the time?

This was referenced Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants