Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Messenger] Use newer version of Beanstalkd bridge library#60040
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
| "pda/pheanstalk":"^4.0", | ||
| "pda/pheanstalk":"^5.1|^7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These are the versions that have thedisconnect() method, which is needed for the reconnect mechanism. Also, v7 supports only PHP 8.3+, while v5 supports PHP 8.1+.
src/Symfony/Component/Messenger/Bridge/Beanstalkd/Transport/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| return$this->client->watchOnly($this->tube)->reserveWithTimeout($this->timeout); | ||
| if ($this->client->watch($this->tube) >1) { | ||
| foreach ($this->client->listTubesWatched()as$tube) { | ||
| if ((string)$tube !== (string)$this->tube) { | ||
| $this->client->ignore($tube); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ThewatchOnly method was removed, this implements similar behavior.
| $job =$this->client->useTube($this->tube)->put( | ||
| $this->client->useTube($this->tube); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Some methods likeuseTube are no longer chainable.
| @@ -503,3 +517,7 @@ public function testSendWithRoundedDelay() | |||
| $connection->send($body,$headers,$delay); | |||
| } | |||
| } | |||
| interface PheanstalkInterfaceextends PheanstalkPublisherInterface, PheanstalkSubscriberInterface, PheanstalkManagerInterface | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ThePheanstalkInterface was split into multiple interfaces. Since the client implements all of them, it's hard to mock. This is a helper interface to make mocking easier.
| try { | ||
| return$command(); | ||
| }catch (ConnectionException) { | ||
| $this->client->disconnect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure what to do about the Psalm errors. Even though thedisconnect() method wasn't added to the interfaces in v5 (to avoid BC breaks), it was added to thePheanstalk class itself, which is what's used here, so the method always exists. I could usemethod_exists(), but that feels redundant. The method was added to the interfaces in v7, but that version requires PHP 8.3.
Thank you@HypeMC. |
e282c2f intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
…alk (HypeMC)This PR was merged into the 7.3 branch.Discussion----------[Messenger] Fix integration with newer versions of Pheanstalk| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | -| License | MITFollow-up to#60040.When I updated the Pheanstalk library, I missed the fact that version 4 internally tracked the [used](https://github.com/pheanstalk/pheanstalk/blob/1459f2f62dddfe28902e0584708417dddd79bd70/src/Pheanstalk.php#L316-L324)/[watched](https://github.com/pheanstalk/pheanstalk/blob/1459f2f62dddfe28902e0584708417dddd79bd70/src/Pheanstalk.php#L329-L337) tube. This was removed in newer versions, so now every call to `useTube()` or `watch()` sends a command to Beanstalkd, leading to unnecessary bandwidth consumption.This PR adds simple tracking logic to avoid redundant calls to those functions.Commits-------b766607 [Messenger] Fix integration with newer version of Pheanstalk
Currently, the Beanstalkd bridge supports version 4 of
pda/pheanstalk, which is no longer maintained. The latest version is v7, and the actively maintained versions appear to be v5 and v7. Version 7 was released just a week after v6, and from what I can tell, v6 is not supported as no patch versions have ever been released for it.This PR bumps support to the maintained versions. I've split it into two commits to make reviewing easier:
Connectionclass.I'm not sure which version of Symfony this should target. It's not exactly a new feature, but the changes are significant.