Add unsubscribe feature for client #196
Conversation
| * @param \Thruway\AbstractSession $session | ||
| * @param \Thruway\Message\ErrorMessage $msg | ||
| */ | ||
| protected function processUnsubscribeError(AbstractSession $session, ErrorMessage $msg) |
There was a problem hiding this comment.
This method should be called processUnsubscribe
There was a problem hiding this comment.
This method is called whenever the router responds with an ERROR message. Why should it be called processUnsubscribe? It follows the same logic as processSubscribeError.
There was a problem hiding this comment.
processUnsubscribeError is correct. Only the broker would handle the UNSUBSCRIBE message.
|
@mbonneau Just took a quick look at this. We should write some tests and also figure out why PHP7 tests are failing. |
src/Thruway/Role/Subscriber.php
Outdated
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Does the spec say anything about what the client should do if there is an error unsubscribing?
There was a problem hiding this comment.
src/Thruway/Role/Subscriber.php
Outdated
| * process unsubscribe | ||
| * @param ClientSession $session | ||
| * @param string $subscriptionId | ||
| * @return boolean |
There was a problem hiding this comment.
I do see that this actually returns promise - the docblock just needs updated
|
@vyk12 This is looking pretty good. I would like to get some tests for it if you have a chance - or let me know and I can see if I can find some time to write some tests. |
| foreach ($this->subscriptions as $key => $subscription) { | ||
| if ($subscription["unsubscribed_request_id"] === $msg->getErrorRequestId()) { | ||
| // reject the promise | ||
| $subscription['unsubscribed_deferred']->reject($msg); |
There was a problem hiding this comment.
Is this deferred used anywhere? I see pieces of this idea in the code but I don't think it is used anywhere - we may be able to get rid of it.
There was a problem hiding this comment.
This deferred is the one that wraps the promise returned by the unsubscribe method, so yes, it is used (and is kind of important.) Or I didn't understand what you meant?
|
@mbonneau I haven't checked yet the way you wrote your phpunit tests, and I have to admit that I am not yet really familiar with it. I think it would be better that you take care of it. However, if you have multiple tests to write, maybe you can write a couple of these and I'll see what I can do to help to write others. By the way, I intend to update the unsubscribe part of the documentation to indicate that it is implemented. I can also update the /Examples/SimpleClient.php and/or the /Examples/ClientExample.php to show how to use it, you tell me. |
|
@mbonneau @davidwdan Any news about this PR? |
|
Hi @vyk12 , I would like to see some tests of this before merging - I will get to work on that when I have a moment so that we can merge this. Thanks |
This Pull Request implements unsubscribe feature for the client. Please let me know if I missed something!