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

Update library/Rediska/PubSub/Channel.php #77

Closed
wants to merge 1 commit into from
Closed

Update library/Rediska/PubSub/Channel.php #77

wants to merge 1 commit into from

Conversation

dimafern
Copy link

Issue #74: unsubscribe error fix
#74

Issue #74: unsubscribe error fix
@@ -459,7 +459,8 @@ protected function _execCommand($command, $channels)
foreach($channels as $channel) {
$execCommand[] = $this->getRediska()->getOption('namespace') . $channel;
}
$connection = $this->_connections->getConnectionByAlias($connectionAlias);
if (!$connection)
Copy link
Author

Choose a reason for hiding this comment

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

$connection is returned by methods earlier and there is no need to search for it again

Copy link
Owner

Choose a reason for hiding this comment

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

Connection from prev foreach may be wrong, if channels from different connections

Copy link
Author

Choose a reason for hiding this comment

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

OK, but the problem is when you unsubscribe the connection is removed from connections list and you can't find it with getConnectionByAlias later.

Copy link
Owner

Choose a reason for hiding this comment

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

Than move $this->_connections->removeChannel($channel) after the getting responses.

@till
Copy link
Collaborator

till commented Oct 10, 2012

Ok, very, very cool. We are getting there!

OK, one addition!

Instead of:

if ($something)
    $foo = $something;

Please do:

if ($something) {
    $foo = $something;
}

Make sure to indent with 4 spaces.

(Aka coding style.)

All you have to do is make those changes in your branch and push again – github will update this PR!

The test case:
Let me think about this, I see there are non yet. But I'll think of something.

@dimafern
Copy link
Author

Made changes, sorry, but we have another pull request now, i did something wrong maybe.

@till
Copy link
Collaborator

till commented Oct 10, 2012

Btw, to get this branch on your localhost:

  1. git clone [email protected]:dimafern/Rediska.git
  2. cd Rediska && git checkout -t origin/patch-1

Then you are on the patch-1-branch – just confirm with git status.

Then do your commits and git push origin patch-1 again.

@shumkov shumkov closed this Aug 7, 2013
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.

3 participants