#1229 Allow return null in kafka Serializer (delete messages aka Tombstones) #1230

Hubbitus · 2022-01-11T23:15:17Z

…es aka Tombstone)

Hubbitus · 2022-01-14T20:35:51Z

There are some errors in automated tests like:


1) Enqueue\Wamp\Tests\Functional\WampConsumerTest::testShouldSendAndReceiveMessage
Error: Call to undefined function GuzzleHttp\Psr7\uri_for()

/mqdev/vendor/ratchet/pawl/src/Connector.php:126
/mqdev/vendor/ratchet/pawl/src/Connector.php:38
/mqdev/vendor/thruway/pawl-transport/src/PawlTransportProvider.php:59
/mqdev/vendor/thruway/client/src/Peer/Client.php:230
/mqdev/pkg/wamp/WampConsumer.php:96
/mqdev/pkg/wamp/Tests/Functional/WampConsumerTest.php:36
/mqdev/pkg/test/RetryTrait.php:26
phpvfscomposer:///mqdev/vendor/phpunit/phpunit/phpunit:97

But that looks like can't be introduced by my change. Could it be possible just restart failed actions?

Steveb-p · 2022-01-14T20:40:31Z

Seems like too high version of guzzle/psr7 is used (2.1.0) and we are using some deprecated functions - or rather, they were not upgraded.

See https://github.com/guzzle/psr7#upgrading-from-function-api.

And yeah, it's outside of the scope of change.

EDIT: It seems to be coming from external library, https://github.com/ratchetphp/Pawl.

EDIT2: ratchet/pawl is installed in version 0.3.5, 0.4.0 is has this "fixed".

Hubbitus · 2022-01-16T13:10:26Z

@Steveb-p, thank you for the answer.

And yeah, it's outside of the scope of change.

So, could you please review the change itself? That is very simple I think.

Steveb-p · 2022-01-16T16:15:28Z

pkg/rdkafka/JsonSerializer.php

+        if (null !== $string) {
+            $data = json_decode($string, true);
+            if (\JSON_ERROR_NONE !== json_last_error()) {
+                throw new \InvalidArgumentException(sprintf('The malformed json given. Error %s and message %s', json_last_error(), json_last_error_msg()));
+            }
+        } else {
+            $data = ['body' => null, 'properties' => null, 'headers' => 'headers'];


Prefer early return for readability:

Suggested change

if (null !== $string) {

$data = json_decode($string, true);

if (\JSON_ERROR_NONE !== json_last_error()) {

throw new \InvalidArgumentException(sprintf('The malformed json given. Error %s and message %s', json_last_error(), json_last_error_msg()));

}

} else {

$data = ['body' => null, 'properties' => null, 'headers' => 'headers'];

if (null === $string) {

return new RdKafkaMessage(null, null, null);

}

$data = json_decode($string, true);

if (\JSON_ERROR_NONE !== json_last_error()) {

throw new \InvalidArgumentException(sprintf('The malformed json given. Error %s and message %s', json_last_error(), json_last_error_msg()));

}

Ok, applied.

Steveb-p · 2022-01-17T01:52:43Z

pkg/rdkafka/JsonSerializer.php

@@ -6,34 +6,30 @@

 class JsonSerializer implements Serializer
 {
-    public function toString(RdKafkaMessage $message): string
+    public function toString(RdKafkaMessage $message): ?string


That's not true, and it would not match with the Stringable interface that is part of PHP8.

See https://www.php.net/manual/en/class.stringable.php.

Suggested change

public function toString(RdKafkaMessage $message): ?string

public function toString(RdKafkaMessage $message): string

But Serializer does not extend Stringable?

And Stringable defines interface public __toString(): string method with two underscores in name, not toString

pkg/rdkafka/Serializer.php

Steveb-p

toString should always return a string, especially since it's based on RdKafkaMessage and can be thought of as an implementation of Stringable.

Otherwise I have no further comments, and it looks good to me.

Hubbitus · 2022-01-19T21:07:03Z

@Steveb-p, Stringable interface defines public __toString(): string method with two underscores in name, not toString. And Serializer does not extend Stringable. Why you are speaking about it?

That is the main change in that pull request. Without it, there is not possible to write into a topic null value. Is not?

Steveb-p · 2022-01-19T21:33:59Z

@Hubbitus I kinda assumed it was, my mistake.

Hubbitus · 2022-01-24T20:16:38Z

@Steveb-p, no problem.
Can we then continue the review please?

Hubbitus · 2022-02-06T14:41:38Z

Guys, could we please continue?
That is a quite trivial change I think. But very important.

Steveb-p · 2022-02-06T19:11:36Z

Guys, could we please continue? That is a quite trivial change I think. But very important.

Actually, it's not trivial. It breaks the contract for the interface, so semver would dictate that it should have a major version bump. Unfortunately the enqueue library, similarly to Symfony, has a shared version across all packages, so it's not exactly acceptable to just bump this one package and not the rest.

If it were to be released as a minor version, it would cause people that update to have broken builds / applications, so there is probably no solution that covers both your need (of a new functionality, kinda) and dependent applications that do have their own serializers.

Do you need this for your own application, or is it for your own library that is re-used by others? If it's an application or internal library, then I suggest you overwrite the dependency with your own fork or rely on your own fork with replace entry, so it "overwrites" the enqueue.

I hope that a new major version of enqueue is released eventually, but given the current state of the package (makasim, the owner, is currently occupied with projects in other languages and is not actively maintaining the lib) I cannot with good conscience promise that. It's something that probably will need to be discussed outside of this PR.

If you need any help with package overwriting, I'll be glad to help.

Hubbitus · 2022-02-22T21:33:50Z

Hello.
Not introducing API/ABI breakage is a very important point, I agree. Probably indeed that can't be made in a minor version update. But what is wrong with creating new major version, list that as incompatible change (probably single), and encouraging others to step to that new version?

Symfony eventually also should update their dependencies. And said after some time (3-12 months) just drop support of current major version.

Thanks for the links about forks, of course, I already was thinking about that, but it looks like just some temporary workaround rather than the solution (in any update I will be forced to do puls, re-bases, and my releases...).

Steveb-p · 2022-02-22T21:41:19Z

@Hubbitus the issue is that enqueue has a shared versioning, so introducing a single break should result in bumps in all packages. Which don't have any breaking changes atm (due to no small part the fact that some are simply not actively maintained).

Personally, I don't mind, but I'm technically not the owner and I cannot make statements if and when a major version bump will be done.

@makasim ?

stale · 2022-04-16T07:15:57Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Hubbitus · 2022-06-14T10:45:03Z

@makasim, could you please look on it? Is it possible to merge that and create a new version?

php-enqueue#1229 Allow return null in kafka Serializer (delete messag…

09afec8

…es aka Tombstone)

Hubbitus force-pushed the master branch from 565698f to 09afec8 Compare January 11, 2022 23:17

Steveb-p reviewed Jan 16, 2022

View reviewed changes

Prefer early return for readability (by PR 1230 review comments)

90192b2

Steveb-p reviewed Jan 17, 2022

View reviewed changes

pkg/rdkafka/Serializer.php Show resolved Hide resolved

Steveb-p reviewed Jan 17, 2022

View reviewed changes

Steveb-p mentioned this pull request Feb 16, 2022

Add support for Symfony 6; drop < 5.1 #1239

Merged

stale bot added the wontfix label Apr 16, 2022

stale bot closed this Apr 27, 2022

php-enqueue deleted a comment from Hubbitus Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1229 Allow return null in kafka Serializer (delete messages aka Tombstones) #1230

#1229 Allow return null in kafka Serializer (delete messages aka Tombstones) #1230

Hubbitus commented Jan 11, 2022

Hubbitus commented Jan 14, 2022

Steveb-p commented Jan 14, 2022 •

edited

Loading

Hubbitus commented Jan 16, 2022

Steveb-p Jan 16, 2022

Hubbitus Jan 16, 2022

Steveb-p Jan 17, 2022 •

edited

Loading

Hubbitus Jan 17, 2022

Hubbitus Jan 17, 2022

Steveb-p left a comment

Hubbitus commented Jan 19, 2022 •

edited

Loading

Steveb-p commented Jan 19, 2022

Hubbitus commented Jan 24, 2022 •

edited

Loading

Hubbitus commented Feb 6, 2022

Steveb-p commented Feb 6, 2022 •

edited

Loading

Hubbitus commented Feb 22, 2022

Steveb-p commented Feb 22, 2022

stale bot commented Apr 16, 2022

Hubbitus commented Jun 14, 2022

	public function toString(RdKafkaMessage $message): ?string
	public function toString(RdKafkaMessage $message): string

#1229 Allow return null in kafka Serializer (delete messages aka Tombstones) #1230

#1229 Allow return null in kafka Serializer (delete messages aka Tombstones) #1230

Conversation

Hubbitus commented Jan 11, 2022

Hubbitus commented Jan 14, 2022

Steveb-p commented Jan 14, 2022 • edited Loading

Hubbitus commented Jan 16, 2022

Steveb-p Jan 16, 2022

Choose a reason for hiding this comment

Hubbitus Jan 16, 2022

Choose a reason for hiding this comment

Steveb-p Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Hubbitus Jan 17, 2022

Choose a reason for hiding this comment

Hubbitus Jan 17, 2022

Choose a reason for hiding this comment

Steveb-p left a comment

Choose a reason for hiding this comment

Hubbitus commented Jan 19, 2022 • edited Loading

Steveb-p commented Jan 19, 2022

Hubbitus commented Jan 24, 2022 • edited Loading

Hubbitus commented Feb 6, 2022

Steveb-p commented Feb 6, 2022 • edited Loading

Hubbitus commented Feb 22, 2022

Steveb-p commented Feb 22, 2022

stale bot commented Apr 16, 2022

Hubbitus commented Jun 14, 2022

Steveb-p commented Jan 14, 2022 •

edited

Loading

Steveb-p Jan 17, 2022 •

edited

Loading

Hubbitus commented Jan 19, 2022 •

edited

Loading

Hubbitus commented Jan 24, 2022 •

edited

Loading

Steveb-p commented Feb 6, 2022 •

edited

Loading