-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Non-transportable envelope items #27245
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
Conversation
Do you have a specific use-case for this? If we really need to add such thing, wouldn't it be better to have a |
Yes, actually, any use-case in userland where async is not required or simply because the middleware reading the item(s) only acts before transport. Requiring the user to implement
It could be a solution, but TBH it feels really weird implementing an interface to opt-out from this, doesn't it? |
That's a fair point. To me, the opt-in interface (as proposed by @Koc) makes more sense than having the |
ogizanagi/symfony#1 merged |
The downside of marker interfaces is that child classes cannot opt-out from them, so that such messages are totally unreusable. |
The question actually is: does the message need to be aware of its "use cases". If yes, then OK. If not, then this might be considered as a leak in the abstraction. I don't have opinion on this, but we should decide. |
Messages dispatchers from where are added the envelope items are aware of the use-cases, not the message. So in userland, envelope items classes would be designed according to these use-cases, and the transport is something that may or may not make sense for them...but wouldn't harm (unless sensitive data are stored in items and sent to a third-party?). The only harm is DX-wise, forcing the end-user to implement it for nothing. |
Can we provide a trait instead, that implements the method by throwing? |
A |
So we would provide a trait that throws a specific exception for |
This missed an update: we cannot actually rely efficiently on an exception thrown from So the opt-in interface would still be the way to go, but as explained above, marker interfaces have drawbacks, considering potential child classes won't be able opt-out. Perhaps not really an issue in our case (I doubt inheritance will be used extensively for envelope items nor opting-out transport for a specific child has real use-cases). Either ways, we've got some time to think about it until 4.2. |
Based on all these discussions, I'm now pretty much convinced that forcing the EnvelopeItem to be serializable is probably the best option. |
@ogizanagi WDYT? Should we close? |
Alright |
This commit was part of the original PR, but was late so reverted to discuss about it in a next PR.
See discussion starting with #26945 (comment).
Not every envelope item needs to go through transport. The
ReceiverMessage
even is a simple marker item on receiver's side, so no point for it being serializable.We could just choose to ignore non-serializable items when sending but as stated by @sroze :
Adding a
EnvelopeItemInterface::isTransportable(): bool
method answers this by requiring to answer the transportable question. DX-wise, it's still one step more rather than justMyItem implements EnvelopeItemInterface
but with far less hassle than having to implement\Serializable
for nothing.Also, a transportable item not implementing
\Serializable
would still be valid for transport (but I would recommend implementing it to keep PHP serialization under control) and not requiring it means less hassle in case we decide/propose an alternate way to serialize items in the future.