Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Fix transporting non-UTF8 payloads by encoding them using base 64#39970
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1b97648 to6fc9e51Comparerimas-kudelis commentedJan 25, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
When you wrote this, did you mean it will increase the size of our messages in the storage, or did you refer to emails that are later sent out? From the code it looks like the former, but your comment almost implies the latter. |
nicolas-grekas commentedJan 25, 2021
yes, that's what I meant. |
| thrownewMessageDecodingFailedException('Encoded envelope should have at least a "body".'); | ||
| } | ||
| if (false ===strpos($encodedEnvelope['body'],'}', -1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't this check a bit errorprone, what about wrapping the base64 encoded content or prefix it with a special key which we can check against later when retrieving the message?
nicolas-grekasJan 25, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think it's error-prone: the php serialization format is likely more stable than this very class.
fabpot commentedJan 25, 2021
Thank you@nicolas-grekas. |
sroze commentedJan 28, 2021
Nice change@nicolas-grekas ! |
Uh oh!
There was an error while loading.Please reload this page.
Replaces#33920
When using the Doctrine transport, sending emails with binary attachments currently requires a custom Messenger serializer because the "body" column is created for UTF-8 only.
In#33920, it is proposed to change the TEXT type to a BLOB. It leaves at least one problem unhandled: the conversion of existing messenger tables.
This PR takes a more conservative approach, by encoding messages to base 64, only if they are non-UTF8.
Compatibility with the existing format is preserved.
The drawback of this approach is that the size of eg email attachments is going to increase by 33% because of the extra encoding. I think this drawback is acceptable for 4.4, and that this PR is the most pragmatic way to make attachments just work.