Skip to content
Browse files

Move command response parsing to the client class.

Connection classes should just handle, convert and return simple Redis
types while parsing and transforming structured replies should be done
by consumers (see Predis\Client or Predis\Transaction\MultiExecContext).

This actually makes more sense considering that parsing a complex response
with the associated command parser may require different actions. As an
example, the result of EXEC is a multibulk that holds the actual responses,
so we really need to parse each one of its elements and we should also
make sure that iterable multibulks are consumed. We already did that
previously, but it was weird knowing that command parsers were applied
by the connection class.

This also moves some duplicated logic away from each connection class
implementation which is a nice bonus.
  • Loading branch information...
1 parent 37e2b54 commit 1b9e10bdd857984bad0e02009470712d209f8b9f @nrk committed Sep 4, 2012
View
20 lib/Predis/Client.php
@@ -217,11 +217,15 @@ public function __call($method, $arguments)
$command = $this->profile->createCommand($method, $arguments);
$response = $this->connection->executeCommand($command);
- if ($response instanceof ResponseErrorInterface) {
- $response = $this->onResponseError($command, $response);
+ if ($response instanceof ResponseObjectInterface) {
+ if ($response instanceof ResponseErrorInterface) {
+ $response = $this->onResponseError($command, $response);
+ }
+
+ return $response;
}
- return $response;
+ return $command->parseResponse($response);
}
/**
@@ -239,11 +243,15 @@ public function executeCommand(CommandInterface $command)
{
$response = $this->connection->executeCommand($command);
- if ($response instanceof ResponseErrorInterface) {
- return $this->onResponseError($command, $response);
+ if ($response instanceof ResponseObjectInterface) {
+ if ($response instanceof ResponseErrorInterface) {
+ $response = $this->onResponseError($command, $response);
+ }
+
+ return $response;
}
- return $response;
+ return $command->parseResponse($response);
}
/**
View
9 lib/Predis/Connection/AbstractConnection.php
@@ -14,7 +14,6 @@
use Predis\ClientException;
use Predis\Helpers;
use Predis\NotSupportedException;
-use Predis\ResponseObjectInterface;
use Predis\Command\CommandInterface;
use Predis\Protocol\ProtocolException;
@@ -126,13 +125,7 @@ public function executeCommand(CommandInterface $command)
*/
public function readResponse(CommandInterface $command)
{
- $reply = $this->read();
-
- if ($reply instanceof ResponseObjectInterface) {
- return $reply;
- }
-
- return $command->parseResponse($reply);
+ return $this->read();
}
/**
View
15 lib/Predis/Connection/WebdisConnection.php
@@ -13,7 +13,6 @@
use Predis\NotSupportedException;
use Predis\ResponseError;
-use Predis\ResponseObjectInterface;
use Predis\Command\CommandInterface;
use Predis\Connection\ConnectionException;
use Predis\Protocol\ProtocolException;
@@ -267,19 +266,11 @@ public function executeCommand(CommandInterface $command)
throw new ConnectionException($this, trim($error), $errno);
}
- $readerState = phpiredis_reader_get_state($this->reader);
-
- if ($readerState === PHPIREDIS_READER_STATE_COMPLETE) {
- $reply = phpiredis_reader_get_reply($this->reader);
-
- if ($reply instanceof ResponseObjectInterface) {
- return $reply;
- }
-
- return $command->parseResponse($reply);
- } else {
+ if (phpiredis_reader_get_state($this->reader) !== PHPIREDIS_READER_STATE_COMPLETE) {
throw new ProtocolException($this, phpiredis_reader_get_error($this->reader));
}
+
+ return phpiredis_reader_get_reply($this->reader);
}
/**
View
47 tests/PHPUnit/ConnectionTestCase.php
@@ -107,7 +107,7 @@ public function testSendingCommandForcesConnection()
$connection = $this->getConnection($profile);
$cmdPing = $profile->createCommand('ping');
- $this->assertTrue($connection->executeCommand($cmdPing));
+ $this->assertSame('PONG', $connection->executeCommand($cmdPing));
$this->assertTrue($connection->isConnected());
}
@@ -119,12 +119,10 @@ public function testExecutesCommandOnServer()
$connection = $this->getConnection($profile);
$cmdPing = $this->getMock($profile->getCommandClass('ping'), array('parseResponse'));
- $cmdPing->expects($this->once())
- ->method('parseResponse')
- ->with('PONG')
- ->will($this->returnValue(true));
+ $cmdPing->expects($this->never())
+ ->method('parseResponse');
- $this->assertTrue($connection->executeCommand($cmdPing));
+ $this->assertSame('PONG', $connection->executeCommand($cmdPing));
}
/**
@@ -150,13 +148,11 @@ public function testReadsCommandFromServer()
$connection = $this->getConnection($profile);
$cmdPing = $this->getMock($profile->getCommandClass('ping'), array('parseResponse'));
- $cmdPing->expects($this->once())
- ->method('parseResponse')
- ->with('PONG')
- ->will($this->returnValue(true));
+ $cmdPing->expects($this->never())
+ ->method('parseResponse');
$connection->writeCommand($cmdPing);
- $this->assertTrue($connection->readResponse($cmdPing));
+ $this->assertSame('PONG', $connection->readResponse($cmdPing));
}
/**
@@ -167,24 +163,20 @@ public function testIsAbleToWriteMultipleCommandsAndReadThemBackForPipelining()
$connection = $this->getConnection($profile);
$cmdPing = $this->getMock($profile->getCommandClass('ping'), array('parseResponse'));
- $cmdPing->expects($this->once())
- ->method('parseResponse')
- ->with('PONG')
- ->will($this->returnValue(true));
+ $cmdPing->expects($this->never())
+ ->method('parseResponse');
$cmdEcho = $this->getMock($profile->getCommandClass('echo'), array('parseResponse'));
$cmdEcho->setArguments(array('ECHOED'));
- $cmdEcho->expects($this->once())
- ->method('parseResponse')
- ->with('ECHOED')
- ->will($this->returnValue('ECHOED'));
+ $cmdEcho->expects($this->never())
+ ->method('parseResponse');
$connection = $this->getConnection();
$connection->writeCommand($cmdPing);
$connection->writeCommand($cmdEcho);
- $this->assertTrue($connection->readResponse($cmdPing));
+ $this->assertSame('PONG', $connection->readResponse($cmdPing));
$this->assertSame('ECHOED', $connection->readResponse($cmdEcho));
}
@@ -195,18 +187,15 @@ public function testSendsInitializationCommandsOnConnection()
{
$connection = $this->getConnection($profile, true);
- $cmdPing = $this->getMock($profile->getCommandClass('ping'), array('parseResponse'));
+ $cmdPing = $this->getMock($profile->getCommandClass('ping'), array('getArguments'));
$cmdPing->expects($this->once())
- ->method('parseResponse')
- ->with('PONG')
- ->will($this->returnValue(true));
+ ->method('getArguments')
+ ->will($this->returnValue(array()));
- $cmdEcho = $this->getMock($profile->getCommandClass('echo'), array('parseResponse'));
- $cmdEcho->setArguments(array('ECHOED'));
+ $cmdEcho = $this->getMock($profile->getCommandClass('echo'), array('getArguments'));
$cmdEcho->expects($this->once())
- ->method('parseResponse')
- ->with('ECHOED')
- ->will($this->returnValue('ECHOED'));
+ ->method('getArguments')
+ ->will($this->returnValue(array('ECHOED')));
$connection->pushInitCommand($cmdPing);
$connection->pushInitCommand($cmdEcho);
View
18 tests/Predis/ClientTest.php
@@ -287,19 +287,27 @@ public function testCreatesNewCommandUsingSpecifiedProfile()
/**
* @group disconnected
*/
- public function testExecuteCommand()
+ public function testExecuteCommandReturnsParsedReplies()
{
- $ping = ServerProfile::getDefault()->createCommand('ping', array());
+ $profile = ServerProfile::getDefault();
+
+ $ping = $profile->createCommand('ping', array());
+ $hgetall = $profile->createCommand('hgetall', array('metavars', 'foo', 'hoge'));
$connection= $this->getMock('Predis\Connection\ConnectionInterface');
- $connection->expects($this->once())
+ $connection->expects($this->at(0))
->method('executeCommand')
->with($ping)
- ->will($this->returnValue(true));
+ ->will($this->returnValue('PONG'));
+ $connection->expects($this->at(1))
+ ->method('executeCommand')
+ ->with($hgetall)
+ ->will($this->returnValue(array('foo', 'bar', 'hoge', 'piyo')));
$client = new Client($connection);
$this->assertTrue($client->executeCommand($ping));
+ $this->assertSame(array('foo' => 'bar', 'hoge' => 'piyo'), $client->executeCommand($hgetall));
}
/**
@@ -351,7 +359,7 @@ public function testCallingRedisCommandExecutesInstanceOfCommand()
$connection->expects($this->once())
->method('executeCommand')
->with($this->isInstanceOf('Predis\Command\ConnectionPing'))
- ->will($this->returnValue(true));
+ ->will($this->returnValue('PONG'));
$profile = $this->getMock('Predis\Profile\ServerProfileInterface');
$profile->expects($this->once())
View
2 tests/Predis/Connection/PhpiredisConnectionTest.php
@@ -83,7 +83,7 @@ public function testExecutesCommandsOnServer()
$cmdRpush = $profile->createCommand('rpush', array('metavars', 'foo', 'hoge', 'lol'));
$cmdLrange = $profile->createCommand('lrange', array('metavars', 0, -1));
- $this->assertTrue($connection->executeCommand($cmdPing));
+ $this->assertSame('PONG', $connection->executeCommand($cmdPing));
$this->assertSame('echoed', $connection->executeCommand($cmdEcho));
$this->assertNull($connection->executeCommand($cmdGet));
$this->assertSame(3, $connection->executeCommand($cmdRpush));
View
2 tests/Predis/Connection/WebdisConnectionTest.php
@@ -130,7 +130,7 @@ public function testExecutesCommandsOnServer()
$cmdRpush = $profile->createCommand('rpush', array('metavars', 'foo', 'hoge', 'lol'));
$cmdLrange = $profile->createCommand('lrange', array('metavars', 0, -1));
- $this->assertTrue($connection->executeCommand($cmdPing));
+ $this->assertSame('PONG', $connection->executeCommand($cmdPing));
$this->assertSame('echoed', $connection->executeCommand($cmdEcho));
$this->assertNull($connection->executeCommand($cmdGet));
$this->assertSame(3, $connection->executeCommand($cmdRpush));

0 comments on commit 1b9e10b

Please sign in to comment.
Something went wrong with that request. Please try again.