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

[1.7.x] Fix serialize #91

Closed
wants to merge 2 commits into from

Conversation

lukecurtis93
Copy link
Contributor

@lukecurtis93 lukecurtis93 commented Apr 6, 2022

Hi there 👋

Thanks for a great package!

When using the default serialize class in the changed file on the PR it actually edits the original variable therefore if you were to send the message twice (for whatever reason) it would serialize twice.. I've attached a screen grab of this happening in the kafka cluster.

You can replicate with the below

use Junges\Kafka\Facades\Kafka;
use Junges\Kafka\Message\Message;

$message = new Message(
    body: ['key' => 'value'],
);

Kafka::publishOn('test-topic')->withMessage($message)->send();
Kafka::publishOn('test-topic')->withMessage($message)->send();

This PR simply clones the message - please let me know if you'd prefer a different approach
Screenshot 2022-04-06 at 14 54 12

@mateusjunges
Copy link
Owner

Hi @lukecurtis93 can you please add a test for this case?

@mateusjunges mateusjunges changed the title Fix serialize [1.7.x] Fix serialize Apr 7, 2022
@mateusjunges
Copy link
Owner

Also, i think we should target v1.6.x for this fix

@lukecurtis93
Copy link
Contributor Author

for sure mate no worries 😊 will update asap

@lukecurtis93 lukecurtis93 changed the base branch from v1.7.x to v1.6.x April 7, 2022 08:52
@lukecurtis93 lukecurtis93 changed the base branch from v1.6.x to v1.7.x April 7, 2022 08:52
@lukecurtis93 lukecurtis93 mentioned this pull request Apr 7, 2022
@mateusjunges
Copy link
Owner

Thank you. I'll close this and merge #92

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.

2 participants