Search code, repositories, users, issues, pull requests...

Copy link

Author

Hubbitus commented Jan 14, 2022

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?

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

Copy link

Contributor

Steveb-p commented Jan 14, 2022 •

edited

Loading

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.

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".

Copy link

Author

Hubbitus commented Jan 16, 2022

@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

Steveb-p reviewed

Jan 16, 2022

pkg/rdkafka/JsonSerializer.php Outdated

Comment on lines 26 to 32

if (null !== $string) {

$data = json_decode($string, true);

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

thrownew \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'];

Copy link

Contributor

@Steveb-p Steveb-p Jan 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

}

Copy link

Author

@Hubbitus Hubbitus Jan 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, applied.


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

90192b2

Steveb-p

Steveb-p reviewed

Jan 17, 2022

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

pkg/rdkafka/JsonSerializer.php

class JsonSerializer implements Serializer

{

public function toString(RdKafkaMessage $message): string

public function toString(RdKafkaMessage $message): ?string

Copy link

Contributor

@Steveb-p Steveb-p Jan 17, 2022 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change

public function toString(RdKafkaMessage $message): ?string

public function toString(RdKafkaMessage $message): string

Copy link

Author

@Hubbitus Hubbitus Jan 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But Serializer does not extend Stringable?

Copy link

Author

@Hubbitus Hubbitus Jan 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Steveb-p

Steveb-p reviewed

Jan 17, 2022

pkg/rdkafka/Serializer.php Show resolved Hide resolved

Steveb-p

Steveb-p reviewed

Jan 17, 2022

Copy link

Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Author

Hubbitus commented Jan 19, 2022 •

edited

Loading

@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?

Copy link

Contributor

Steveb-p commented Jan 19, 2022

@Hubbitus I kinda assumed it was, my mistake.

Copy link

Author

Hubbitus commented Jan 24, 2022 •

edited

Loading

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

Copy link

Author

Hubbitus commented Feb 6, 2022

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

Add support for Symfony 6; drop < 5.1 #1239

Copy link

Contributor

Steveb-p commented Feb 6, 2022 •

edited

Loading

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.

@Steveb-p Steveb-p mentioned this pull request

Feb 16, 2022

Merged

Copy link

Author

Hubbitus commented Feb 22, 2022

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...).

Copy link

Contributor

Steveb-p commented Feb 22, 2022

@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

Copy link

stale bot commented Apr 16, 2022

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.

@stale stale bot added the wontfix label

Apr 16, 2022

@stale stale bot closed this

Apr 27, 2022

@php-enqueue php-enqueue deleted a comment from Hubbitus

Jun 8, 2022