Skip to content

[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

Closed
wants to merge 2 commits into from
Closed

[Messenger] Non-transportable envelope items #27245

wants to merge 2 commits into from

Conversation

ogizanagi
Copy link
Contributor

@ogizanagi ogizanagi commented May 12, 2018

Q A
Branch? 4.1
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

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 :

it could be a source of potential confusion and "tricky bugs" (i.e. the middleware X doesn’t “work” anymore because I forgot to add the Serializable on my EnvelopeItem)

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 just MyItem 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.

@sroze
Copy link
Contributor

sroze commented May 13, 2018

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 NotSerializableEnvelopeItemInterface, so it's explicit and we keep it serializable "by default"?

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone May 13, 2018
@ogizanagi
Copy link
Contributor Author

Do you have a specific use-case for this?

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 \Serializable when it's strictly unnecessary looks like a poor DX to me.

wouldn't it be better to have a NotSerializableEnvelopeItemInterface, so it's explicit and we keep it serializable "by default"?

It could be a solution, but TBH it feels really weird implementing an interface to opt-out from this, doesn't it?
Another solution is not considering the issue you've mentioned in the description a DX issue, but rather a documentation one and segregate properly envelope items from the notion of transport, as suggested precisely by @Koc in #26945 (comment).
If you agree, I'll merge his PR in this branch.

@sroze
Copy link
Contributor

sroze commented May 14, 2018

Requiring the user to implement \Serializable when it's strictly unnecessary looks like a poor DX to me.

That's a fair point. To me, the opt-in interface (as proposed by @Koc) makes more sense than having the isTransportable() without any kind of contract on how it is going to be transported (which is the case of the interface, because of the \Serializable). If we have too many issues caused by the subtility of some items not being serialized, then we can force it in 4.2 anyway :)

@ogizanagi
Copy link
Contributor Author

ogizanagi/symfony#1 merged

@nicolas-grekas
Copy link
Member

The downside of marker interfaces is that child classes cannot opt-out from them, so that such messages are totally unreusable.
From afar, forcing ppl to implement serializable may make sense to me: Messenger are for things that can pass through any bus, local or remote.
If you do not want to be able to pass through them, use event dispatcher.
Does that make sense?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 15, 2018

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.

@ogizanagi
Copy link
Contributor Author

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.

@nicolas-grekas
Copy link
Member

Can we provide a trait instead, that implements the method by throwing?

@ogizanagi
Copy link
Contributor Author

ogizanagi commented May 15, 2018

A Non(Transportable|Serializable)Exception to be caught in SendMessageMiddleware or Serializer to ignore those items?
Could be an acceptable solution indeed.
@sroze : WDYT?

@sroze
Copy link
Contributor

sroze commented May 17, 2018

So we would provide a trait that throws a specific exception for serialize and unserialize. This exception would be caught and ignored by the Serializer (IMHO) (logged as info?). That trait could be called NonTransportableMessageTrait. I like it (much more than the serialization opt-in)

@sroze sroze modified the milestones: 4.1, next May 21, 2018
@ogizanagi
Copy link
Contributor Author

ogizanagi commented May 31, 2018

This missed an update: we cannot actually rely efficiently on an exception thrown from serialize, as we serialize the whole array of Envelope::all() items (or that would means either serializing twice or manually linearizing the array in PHP format with items already serialized separately).

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).
Otherwise, the first commit of this PRs (EnvelopeItemInterface::isTransportable(): bool) allows opting-out.

Either ways, we've got some time to think about it until 4.2.

@sroze
Copy link
Contributor

sroze commented May 31, 2018

Based on all these discussions, I'm now pretty much convinced that forcing the EnvelopeItem to be serializable is probably the best option.

@sroze
Copy link
Contributor

sroze commented Jul 20, 2018

@ogizanagi WDYT? Should we close?

@ogizanagi
Copy link
Contributor Author

Alright

@ogizanagi ogizanagi closed this Jul 20, 2018
@ogizanagi ogizanagi deleted the messenger/non_transportable branch October 31, 2018 17:40
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants