[Messenger] Remove base64_encode & use addslashes #30957

weaverryan · Apr 7, 2019

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	already covered by existing issue

In #30814, we base64_encoded messages because some transports (specifically DoctrineTransport + Postgresql & SQS) do not allow binary data.

The downside is that the messages become unreadable, which makes it much less convenient to debug your messages with 3rd party monitoring tools, for example.

This PR replaces base64_encode with addslashes. Another alternative (that I first tried in this PR) was to use a blob type, which Drupal does in its code (https://www.drupal.org/project/drupal/issues/690746). But, it still meant that binary data could cause problems with other transports, like SQS.

I also put all the serializer config under a nice, neat serializer key under messenger.

Best seen with ?w=1.

Cheers!

stof · Apr 7, 2019

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

-            <xsd:element name="default-serializer" type="xsd:string" minOccurs="0" />
-            <xsd:element name="symfony-serializer" type="messenger_symfony_serializer" minOccurs="0" />
-            <xsd:element name="encoder" type="xsd:string" minOccurs="0" />
-            <xsd:element name="decoder" type="xsd:string" minOccurs="0" />


why are the encoder and decoder gone ?

I can't find them referenced anywhere - there is no encoder or decoder in the Configuration class. I think these were incorrectly not removed from some earlier change to messenger.

stof · Apr 7, 2019

src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php

-        $serializeEnvelope = base64_decode($encodedEnvelope['body']);
+        $serializeEnvelope = $encodedEnvelope['body'];
+
+        if (true === $this->base64Encode) {


I suggest using if ($this->base64Encode) as it is already a boolean anyway

nicolas-grekas

once stof's comments are addressed

nicolas-grekas · Apr 8, 2019

Another proposal would be to always use addslashes/stripslashes on the serialized payload.
That would allow removing the option - ie less complexity. WDYT?

weaverryan · Apr 8, 2019

Another proposal would be to always use addslashes/stripslashes on the serialized payload.
That would allow removing the option - ie less complexity. WDYT?

I think this does fix the problem. It would not allow message objects themselves to contain binary data, but probably that's not supported? Here is what SQS says about character sets:

A message can include only XML, JSON, and unformatted text. The following Unicode characters are allowed:

#x9 | #xA | #xD | #x20 to #xD7FF | #xE000 to #xFFFD | #x10000 to #x10FFFF

Is addslashes sufficient?

lyrixx · Apr 8, 2019

That would allow removing the option - ie less complexity. WDYT?

I don't like this option. I often copy/paste message from/to AMQP. More over I often have different language between my producer and my consumer. (But I'm not sure the messenger component is designed for such use cases).

Instead of an option, why don't you add Transport::needEscaping() ?

weaverryan · Apr 8, 2019

I often copy/paste message from/to AMQP.

What's the format you usually use? serialized PHP or JSON?

More over I often have different language between my producer and my consumer. (But I'm not sure the messenger component is designed for such use cases).

It absolutely is. But in that case, you'd be using the Symfony serializer (JSON) or something custom. Indeed the PhpSerializer is (mostly) only for the situation where you plan to send and consume from your Symfony app.

Instead of an option, why don't you add Transport::needEscaping() ?

That's an interesting idea. In fact, the transport is responsible for calling the serializer... and so it could even pass this as an option to encode() and decode()

nicolas-grekas · Apr 8, 2019

It would not allow message objects themselves to contain binary data

addslashes encodes \0, ', " and \

More over I often have different language between my producer and my consumer

then you're not using the PhpSerializer - my comment applies only to it.

we could also and/or alternatively define a new messenger.transport.base64_encoded_php_serializer service instead of adding a new option

weaverryan · Apr 8, 2019

we could also and/or alternatively define a new messenger.transport.base64_encoded_php_serializer service instead of adding a new option

I like this way.

Instead of an option, why don't you add Transport::needEscaping() ?

On 2nd thought, this might be hard to implement, as this whole escaping thing only applies if you're using the PhpSerializer... and the user may or may not be using it. So, it would be a weird transport option that only applies sometimes.

lyrixx · Apr 8, 2019

What's the format you usually use? serialized PHP or JSON?

JSON of course :)

weaverryan · Apr 10, 2019

Ready for review again - I've updated the description, etc. This changes to use addslashes instead of base64_encode(). This only applies to the PhpSerializer.

nicolas-grekas

messenger.transport.base64_encoded_php_serializer

for another PR when needed?

nicolas-grekas · Apr 15, 2019

src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php

-        if (false === $serializeEnvelope) {
-            throw new MessageDecodingFailedException('The "body" key could not be base64 decoded.');
-        }
+        $serializeEnvelope = \stripslashes($encodedEnvelope['body']);


\ should be removed

nicolas-grekas · Apr 15, 2019

src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php

@@ -44,8 +40,10 @@ public function decode(array $encodedEnvelope): Envelope
     */
    public function encode(Envelope $envelope): array
    {
+        $body = \addslashes(serialize($envelope));


\ should be removed

…with addslashes

weaverryan · Apr 15, 2019

messenger.transport.base64_encoded_php_serializer
for another PR when needed?

Yep, exactly. It's trivial anyways - you could even decorate the PhpSerializer

Ready to go!

fabpot · Apr 15, 2019

Thank you @weaverryan.

…verryan) This PR was merged into the 4.3-dev branch. Discussion ---------- [Messenger] Remove base64_encode & use addslashes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | already covered by existing issue In #30814, we base64_encoded messages because some transports (specifically DoctrineTransport + Postgresql & SQS) do not allow binary data. The downside is that the messages become unreadable, which makes it much less convenient to debug your messages with 3rd party monitoring tools, for example. This PR replaces base64_encode with addslashes. Another alternative (that I first tried in this PR) was to use a blob type, which Drupal does in its code (https://www.drupal.org/project/drupal/issues/690746). But, it still meant that binary data could cause problems with other transports, like SQS. I also put all the serializer config under a nice, neat `serializer` key under messenger. Best seen with `?w=1`. Cheers! Commits ------- 70b448d Reorganizing messenger serializer config and replacing base64_encode with addslashes

carsonbot added Status: Needs Review Messenger Feature labels Apr 7, 2019

weaverryan added this to the next milestone Apr 7, 2019

weaverryan force-pushed the messenger-blob-db-type branch from ed0775f to 1900a1a Compare April 7, 2019 08:29

weaverryan mentioned this pull request Apr 7, 2019

[Messenger] Making it Shine #30540

Closed

36 tasks

stof reviewed Apr 7, 2019

View reviewed changes

nicolas-grekas approved these changes Apr 7, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 7, 2019

weaverryan force-pushed the messenger-blob-db-type branch from 1900a1a to b9530f5 Compare April 8, 2019 12:59

weaverryan force-pushed the messenger-blob-db-type branch from b9530f5 to c12b8ac Compare April 10, 2019 22:34

weaverryan changed the title ~~[Messenger] Remove base64_encode, but make optional~~ [Messenger] Remove base64_encode & use addslashes Apr 10, 2019

nicolas-grekas approved these changes Apr 15, 2019

View reviewed changes

Reorganizing messenger serializer config and replacing base64_encode …

70b448d

…with addslashes

weaverryan force-pushed the messenger-blob-db-type branch from c12b8ac to 70b448d Compare April 15, 2019 14:26

fabpot approved these changes Apr 15, 2019

View reviewed changes

fabpot merged commit 70b448d into symfony:master Apr 15, 2019

weaverryan deleted the messenger-blob-db-type branch April 18, 2019 10:42

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

weaverryan mentioned this pull request Nov 6, 2019

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

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

[Messenger] Remove base64_encode & use addslashes #30957

[Messenger] Remove base64_encode & use addslashes #30957

Uh oh!

Conversation

weaverryan commented Apr 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof Apr 7, 2019

Choose a reason for hiding this comment

Uh oh!

weaverryan Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

stof Apr 7, 2019

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Apr 8, 2019

Uh oh!

weaverryan commented Apr 8, 2019

Uh oh!

lyrixx commented Apr 8, 2019

Uh oh!

weaverryan commented Apr 8, 2019

Uh oh!

nicolas-grekas commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan commented Apr 8, 2019

Uh oh!

lyrixx commented Apr 8, 2019

Uh oh!

weaverryan commented Apr 10, 2019

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Apr 15, 2019

Uh oh!

fabpot commented Apr 15, 2019

Uh oh!

Uh oh!

weaverryan commented Apr 7, 2019 •

edited

Loading

nicolas-grekas commented Apr 8, 2019 •

edited

Loading