[Messenger] Store message body as a blob object, not text

[rimas-kudelis]

Q	A
Branch?	4.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #33913
License	MIT

This fixes #33913 for me. After changing the column type, I could send emails with attachments via messenger with no problems.

[rimas-kudelis]

Is there anything that prevents you from accepting this patch? I really want to see this in 4.4 by the time it comes out.

[Tobion]

While I think this could be right thing to do and would work with mysql, I'm pretty sure it breaks for other database systems. See for example the PdoSessionStorage that uses BLOB and has different queries for different vendors, e.g.

symfony/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

Lines 817 to 821 in d056c17

	case 'oci':
	$data = fopen('php://memory', 'r+');
	fwrite($data, $sessionData);
	rewind($data);
	$sql = "UPDATE $this->table SET $this->dataCol = EMPTY_BLOB(), $this->lifetimeCol = :expiry, $this->timeCol = :time WHERE $this->idCol = :id RETURNING $this->dataCol into :data";

[Tobion]

Please pass the blob type wherever the body is passed to a query, e.g.

symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

Line 130 in 1a573c3

null,

You need to rebase to get the latest changes

This change also deserves a changelog and upgrade entry and description how to migrate the existing table.

[stof]

As this is a Doctrine-based implementation, DBAL can abstract the difference for us. But it would require setting the parameter type when binding the body param (the default behavior would set it as a string parameter, and so won't handle the different behavior needed for blobs in some platforms)

[Tobion]

I couldn't find where doctrine handles the pdo oci workaround I listed above. It seems that is exactly not implemented in dbal: https://github.com/doctrine/dbal/blob/80a45c6d7e7dba7d0c133bf06724dcf2be6ab5e9/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php#L17-L20

If doctrine can handle the other edge cases, I'm fine. But IMO we should do this change in 4.4, not in 4.3 as a bug fix. And document the table upgrade with mysql as an example.

[rimas-kudelis]

Nice to see some traction after the wait. So what exactly should I change for the patch to be accepted? Rimas Sent from my mobile phone. Please excuse my brevity and potential typos.

…

On 2019 m. lapkričio 5 d. 18:15:47 GMT+02:00, Christophe Coevoet ***@***.***> wrote: As this is a Doctrine-based implementation, DBAL can abstract the difference for us. But it would require setting the parameter type when binding the body param (the default behavior would set it as a string parameter, and so won't handle the different behavior needed for blobs in some platforms) -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #33920 (comment)

[weaverryan]

There is some history behind the decision to do text. Specifically, BLOB columns (iirc) in MySQL are not human-readable, which makes the system more to debug and understand. There was effort during 4.3 to try to keep messages readable.

In #30957, I first played with using a BLOB column (Drupal does this). But due to lack of readability and that some other transports might (?) have problems with binary data (I think SQS only allows a few binary characters).

I propose 2 alternate solutions:

A) We make it possible (via configuration) to use the PhpSerializer but base64_encode (and base64_decode) the data. Then it's binary safe.

B) This solution, but make users need to opt into it.

[rimas-kudelis]

I've rebased and hinted blob type, as requested by @Tobion . I don't mind this going straight to 4.4 since I've already switched to that myself.

@weaverryan regarding BLOB types being not viewable: I can view these columns just fine in MySQL Workbench (although you have to click "Open Value in Editor" in the context menu for the field), so I don't think it's a problem. In general, any solution is fine by me, I just picked this one because it was the lowest hanging fruit and it fixed the bug which prevented the scenario documented in Symfony docs from working.

[stof]

Looks like you messed a rebase. You branch seems to be based on master, not on 4.3

[weaverryan]

What impact would this have when users are upgrading? I suppose no change to their db table would be made (because setup() will not be called again)? That's probably fine... but what about the executeQuery() call that changes the type to Type::BLOB for the body? What happens if the column is not a blob?

[rimas-kudelis]

@weaverryan I can't answer the question about what would happen, but I could add a README entry about the need for manual schema update, as suggested by @Tobion above.

[toooni]

As @weaverryan already suspected, the doctrine transport isn't the only one having issues with this (#35137).

IMO, this means that it would be best if this issue would be solved in serializing with:
A.) Adding a parameter to PhpSerializer to choose either readability (addslashes) or functionality with binary data (base64_encode).
B.) Creating a seperate SafeSerializer which uses base64_encode instead of addslashes.
C.) Revert the solution with addslashes from #30957 and lose readability.

[nicolas-grekas]

What's the next step here?

[Tobion]

This is the right thing to do. But someone needs to test if it keeps working for people that still have the text type. So can you insert non-binary data to the body column if it is of type text (before this change)? Otherwise this PR would be a bc break.

[ricohumme]

I came across this discussion and saw it hadn't been touched in a while. My current quick fix was altering the schema to support longblob, as blob wasn't cutting it.
In the meantime I have been thinking about this, and where things go wrong in this scenario is when binary data is being serialized. I therefor vote for the option to base64 en-/decode this data.
However I don't think the messenger component is responsible for this, but in this case the mailer component.
What is your opinion on this?

[toooni]

@ricohumme I agree. But I am not sure on what to do (as mentioned in my comment #33920 (comment)):

• Having multiple serializers
• Revert addslashes/stripslashes to base64_encode/base64_decode in PhpSerializer
• Add a configurable parameter to PhpSerializer

But I am sure whatever solution will be chosen, base64 en-/decoding should be the default behaviour here.

[rimas-kudelis]

I disagree. IMO, if Messenger accepts a message, it's Messenger's responsibility to handle it properly, and that includes transforming it if and when necessary. Mailer should not care about Messenger's internals, nor should Messenger care about Mailer's.

Let's say Mailer is not the only component that has objects with binary data and sends them in messages. Asking each such component to prepare data so that Messenger can safely transport it doesn't look like a good idea to me, assuming that Messenger can actually do all that itself when it's necessary.

[toooni]

@rimas-kudelis The PhpSerializer is part of the messenger component. So we basically mean the same ;)

Edit: Sorry, I misread the part about which component should be responsible for this. I think it should be the job of the messenger component.

[ricohumme]

The class Symfony\Component\Mime\RawMessage implements \Serializable, so I believe it should be a responsibility of the message. Before serializing the binary part of the message, implemented in Symfony\Component\Mime\Email, it should be base64_encoded. There is already a __serialize method in place, but it does nothing with the contents of an attachment, except fetch it when it is a stream.

[rimas-kudelis]

@ricohumme isn't it already serializable though? IMO the problem here is not about missing slashes, but about raw bytes which aren't defined in a particular character set.

[ricohumme]

What are you referring to with: isn't it already serializable though?

[toooni]

I am using a solution where the messenger component uses a SafeSerializer (copy of PhpSerializer with base64 en-/decoding) in the messenger component for several months in production now. I am sending emails with attachments going through messenger (Redis Transport) and everything works fine. I do not think there is anything wrong with the current mailer component implementation.

[rimas-kudelis]

@ricohumme

What are you referring to with: isn't it already serializable though?

As you said, the class already implements \Serializable, which means, it already fulfills the contract. It could escape attachments, but it doesn't have to.

[ricohumme]

@rimas-kudelis

As you said, the class already implements \Serializable, which means, it already fulfills the contract. It could escape attachments, but it doesn't have to.

Correct, but the attachment containing raw data can't be serialized to the database

@toooni I think the use of a SafeSerializer is beneficial here, but the message in the database is no longer readable what so ever. That is why my suggestion is only to apply the base64 en-/decoding to the attachment section containing raw data.

Otherwise you would need to perform decoding on the column first in order to perform any search on it, which could be heavy for the database when having a lot of messages, specifically nasty if they have failed. Or should perform everything on console using the messenger:failed command.

[toooni]

@ricohumme The issue is that we do not know how each transport behaves. AWS SQS for example has issues with serialized data from PHP because of some characters. I also had issues with the redis transport.
I do understand that readability is a desirable feature. But IMO working out of the box in (almost) every case has a higher priority. A human-readable serializer could still be a configurable alternative.

[ricohumme]

@toooni agreed. Then SafeSerializer is the way to go :)

[rimas-kudelis]

Let's also not forget that a message can be stored in a BLOB-type field in its pristine form. It remains readable in that case, assuming your tool of choice allows viewing BLOB field content without too much hassle.

[ricohumme]

Like a said in a previous post, when pushing the data I had in a blob, which was one attachment, it failed because the data being posted was about 20 mb. So I used a longblob instead, as medium blob was selling short in that department as well.
For reference:
Blob = max 64 kb
Mediumblob = max 16 mb
Longblob = max 4 gb

[rimas-kudelis]

Sure, sure. I meant the whole category of BLOB types, not the BLOB type specifically.

[nicolas-grekas]

I propose another approach in #39970, please have a look.

We've just been hit by this issue when sending PDFs, and having to write a custom serializer for Messenger is just bad DX. We need to solve this!

[nicolas-grekas]

Thank you @rimas-kudelis

[rimas-kudelis]

Thank you for doing this properly! :)