Skip to content

remove base64_encode/decode of the payload.#78

Open
Sergio-Ben wants to merge 1 commit into
kainxspirits:masterfrom
Sergio-Ben:master
Open

remove base64_encode/decode of the payload.#78
Sergio-Ben wants to merge 1 commit into
kainxspirits:masterfrom
Sergio-Ben:master

Conversation

@Sergio-Ben
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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