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

remove base64_encode/decode of the payload. #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sergio-Ben
Copy link

Google lib, which is used underneath is already properly encoding/decoding the payload. There is no need to do it twice.
Here is some screenshots with the proofs :)
Feel free to dive into the google pub/sub lib and check it on your own :)

Screenshot 2024-03-26 at 15 40 07

As it is the non-compatible change, i suggest bumping up major version during releasing

Google pub/sub lib already doing this.
@kainxspirits
Copy link
Owner

Thank you for the contribution.

Even if it not a huge library, it will break for existing users (there is no major version yet). I feel like it would be better to use the encode property of the Google lib in order to make the encoding optional.

@Sergio-Ben
Copy link
Author

@kainxspirits
The problem here is the encode property is something you cannot set from the application level.

Screenshot 2024-05-07 at 09 48 21

It's just an internal flag, because Google pub/sub lib could use 2 types of transport, http or grpc. And encode will always be true for the default http transport.

Also with current approach the message size is much bigger than expected. Encoding message with base64 resulting in increasing payload size by ~ 33%. With the current approach it's even bigger $res = ($mesSize * 1.33) * 1.33
So double encoding will produce ~ 76% bigger payload from original size.

Also, it's worth mentioning the potential release version.

[https://getcomposer.org/doc/articles/versions.md#caret-version-range-]
For pre-1.0 versions it also acts with safety in mind and treats ^0.3 as >=0.3.0 <0.4.0 and ^0.0.3 as >=0.0.3 <0.0.4.

So, even if the next version will be 0.10.0 the composer install command won't fetch the latest version.

Obviously, the release is up to you. Just wanted to highlight the problem I found using this lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants