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 Doctrine] Support for igbinary serializer and binary message body column #52877

Closed

Conversation

FluffyDiscord
Copy link

@FluffyDiscord FluffyDiscord commented Dec 3, 2023

Q A
Branch 7.1, but here you say that new features should be targetting 6.4, so I am not sure
Bug fix no
New feature yes
Deprecations no
Issues #52872
License MIT

Description

This PR introduces alternative Messenger Transport, the igbinary serializer Symfony\Component\Messenger\Transport\Serialization\IgbinarySerializer and deepens the support by also modifying Symfony\Component\Messenger\Bridge\Doctrine\Transport\Connection to support binary column type whenever this serializer is enabled.

The igbinary serializer is by default 1-5 times faster and it's output is around 30-120% shorter than PHP's build-in serialize. Added serializer was also stripped of the base64 pass both on encode and decode to further speed up the conversion. To support binary message body we also need to modify the Doctrine transport connection, so it works with binary column type instead of text.

Every DBAL supported database except Sqlite supports binary columns and installing igbinary extension is easy as installing alteady built php-igbinary package from repos and in other cases pecl install igbinary can be used.

With this PR tests might be broken as they now require ext-igbinary to be installed - maybe this can be skipped if it's not installed?

Questions

  1. I was not sure how to pass the information to Symfony\Component\Messenger\Bridge\Doctrine\Transport\Connection so it knows if it should use binary or text column type for body without making BC. Please let me know if the current approach is fine or not.
  2. How do I fix the psalm error about non existent Doctrine\DBAL\Platforms\SqlitePlatform? Do I need to use class_implements($platform) with in_array() or is there a better solution? I need to check whenever the used platform is Sqlite to throw exception.

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@FluffyDiscord FluffyDiscord marked this pull request as ready for review December 3, 2023 19:47
@carsonbot carsonbot added this to the 7.1 milestone Dec 3, 2023
@nicolas-grekas
Copy link
Member

There's one catch with igbinary: it's not really well maintained (or better: I'd say it misses contributors)
See #52391

I'm not sure enabling more igbinary use cases is a good idea until issues like igbinary/igbinary#273 are resolved.

@FluffyDiscord
Copy link
Author

FluffyDiscord commented Dec 4, 2023

The main usage for this is to serialize Messenger message, which should not really be an issue in this case, you always initialize properties and you don't have any business logic in them, they are just a "fancy" data payloads. The issue you are referring to is a different situation.

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.

None yet

3 participants