Skip to content
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

[Messenger] Remove base64_encode & use addslashes #30957

Merged
merged 1 commit into from Apr 15, 2019

Conversation

Projects
None yet
6 participants
@weaverryan
Copy link
Member

weaverryan commented 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!

@weaverryan weaverryan added this to the next milestone Apr 7, 2019

@weaverryan weaverryan force-pushed the weaverryan:messenger-blob-db-type branch from ed0775f to 1900a1a Apr 7, 2019

@weaverryan weaverryan referenced this pull request Apr 7, 2019

Open

[Messenger] Making it Shine #30540

26 of 35 tasks complete
<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" />

This comment has been minimized.

Copy link
@stof

stof Apr 7, 2019

Member

why are the encoder and decoder gone ?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 8, 2019

Author Member

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.

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

This comment has been minimized.

Copy link
@stof

stof Apr 7, 2019

Member

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

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

once stof's comments are addressed

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented 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

This comment has been minimized.

Copy link
Member Author

weaverryan commented 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?

@weaverryan weaverryan force-pushed the weaverryan:messenger-blob-db-type branch from 1900a1a to b9530f5 Apr 8, 2019

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented 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

This comment has been minimized.

Copy link
Member Author

weaverryan commented 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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented 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

This comment has been minimized.

Copy link
Member Author

weaverryan commented 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

This comment has been minimized.

Copy link
Member

lyrixx commented Apr 8, 2019

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

JSON of course :)

@weaverryan weaverryan force-pushed the weaverryan:messenger-blob-db-type branch from b9530f5 to c12b8ac Apr 10, 2019

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

@weaverryan

This comment has been minimized.

Copy link
Member Author

weaverryan commented 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
Copy link
Member

nicolas-grekas left a comment

messenger.transport.base64_encoded_php_serializer

for another PR when needed?

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 15, 2019

Member

\ should be removed

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 15, 2019

Member

\ should be removed

@weaverryan weaverryan force-pushed the weaverryan:messenger-blob-db-type branch from c12b8ac to 70b448d Apr 15, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

weaverryan commented 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

fabpot approved these changes Apr 15, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Apr 15, 2019

Thank you @weaverryan.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 15, 2019

feature #30957 [Messenger] Remove base64_encode & use addslashes (wea…
…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

@weaverryan weaverryan deleted the weaverryan:messenger-blob-db-type branch Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.