php-enqueue / enqueue-dev

Message Queue, Job Queue, Broadcasting, WebSockets packages for PHP, Symfony, Laravel, Magento. DEVELOPMENT REPOSITORY - provided by Forma-Pro
https://enqueue.forma-pro.com/
MIT License
2.17k stars 430 forks source link

Message body of arbitrary type #831

Open Sevavietl opened 5 years ago

Sevavietl commented 5 years ago

First of all, thank you for the great packages. They helped me and my team a lot in working with such transports as Kafka and Pub/Sub. I think this library is a must-have for message-oriented PHP app.

This issue follows the issue from queue-interop/queue-interop. In a couple of words, now the body of a message is restricted to string. From the source code, I can divide transport into three categories (this taxonomy sometimes based only on the current implementation):

From what I can see all of the implementations (except amqp, why read later on) can be made to use serializable body. It is properly done in rdkafka, redis and wamp: context accepts serializer that is used to serialize/deserialize the message.

The only one that a bit tricky is ampq. You can set the content type of a message in the headers, so it will provide overhead to manage serialization and setting proper content type -- as a user can provide serializer of one type and header of another. This should be validated, that brings some complications, so using a string here is ok, I guess.

Just as a proof of concept I have skipped type hint in interfaces (please, see Sevavietl/queue-interop) and created StringBodyOnlyTrait to restrict implementation (this breaks LSP, although). Then I have altered enqueue packages to use new interfaces and made tests pass (please, see Sevavietl/enqueue-dev). I can create a PR from my fork if you want, just do not know if my implementation is what you want.

From my point of view, this is a minimal amount of work to provide the ability to use arbitrary body type. But in the long run, I'd improve all packages to use a serializer (again, maybe except amqp).

Thank you in advance.

Steveb-p commented 5 years ago

@Sevavietl could you please open a draft PR, so changes made would be more easily accessible? :) You know most people are lazy :)

I'm pretty sure Symfony recently changed their Symfony Messenger component to rely on php's built-in serializer unless a specific serializer instance is passed. Their reasoning was that in most cases built-in serializer is sufficient, especially when dealing with intra-app communication or communication between PHP-only applications.

see: https://symfony.com/blog/new-in-symfony-4-3-native-php-serialization-for-messenger.

The original reason why we did this was so that we could export "generic JSON", in case we wanted other workers to consume the messages, no matter if they used Symfony, PHP or any other programming language and technology. However, this is not the common use case and it was complicating things unnecessarily.

In Symfony 4.3, we fixed this problem by switching the serialization to a new class called PhpSerializer which uses PHP's native serialize() and unserialize() to serialize messages to a transport.

Obviously it's not mandatory to take the same approach in enqueue (Makasim seems to have pretty bad opinion of some design choices made for Messenger component).

makasim commented 5 years ago

I've opened PRs. Hope @Sevavietl dont mind.

https://github.com/queue-interop/queue-interop/pull/28, https://github.com/php-enqueue/enqueue-dev/pull/846.

I will review PRs later this week. Thanks for your work @Sevavietl

Obviously it's not mandatory to take the same approach in enqueue (Makasim seems to have pretty bad opinion of some design choices made for Messenger component).

True, good for starters but once you grow you hit the wall

Sevavietl commented 5 years ago

@makasim thank you.

should have done this myself, but had had a lot of work at work)

will try to make my time now, because I really want to help (as much as I can) such a great project

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.