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.

lib/Predis/Client.php

@@ -217,11 +217,15 @@ class Client implements ClientInterface
         $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 @@ class Client implements ClientInterface
     {
         $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);
     }
 
     /**

lib/Predis/Connection/AbstractConnection.php

@@ -14,7 +14,6 @@ namespace Predis\Connection;
 use Predis\ClientException;
 use Predis\Helpers;
 use Predis\NotSupportedException;
-use Predis\ResponseObjectInterface;
 use Predis\Command\CommandInterface;
 use Predis\Protocol\ProtocolException;
 
@@ -126,13 +125,7 @@ abstract class AbstractConnection implements SingleConnectionInterface
      */
     public function readResponse(CommandInterface $command)
     {
-        $reply = $this->read();
-
-        if ($reply instanceof ResponseObjectInterface) {
-            return $reply;
-        }
-
-        return $command->parseResponse($reply);
+        return $this->read();
     }
 
     /**

lib/Predis/Connection/WebdisConnection.php

@@ -13,7 +13,6 @@ namespace Predis\Connection;
 
 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 @@ class WebdisConnection implements SingleConnectionInterface
             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);
     }
 
     /**

tests/PHPUnit/ConnectionTestCase.php

@@ -107,7 +107,7 @@ abstract class ConnectionTestCase extends StandardTestCase
         $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 @@ abstract class ConnectionTestCase extends StandardTestCase
         $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 @@ abstract class ConnectionTestCase extends StandardTestCase
         $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 @@ abstract class ConnectionTestCase extends StandardTestCase
         $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 @@ abstract class ConnectionTestCase extends StandardTestCase
     {
         $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);

tests/Predis/ClientTest.php

@@ -287,19 +287,27 @@ class ClientTest extends StandardTestCase
     /**
      * @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 @@ class ClientTest extends StandardTestCase
         $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())

tests/Predis/Connection/PhpiredisConnectionTest.php

@@ -83,7 +83,7 @@ class PhpiredisConnectionTest extends ConnectionTestCase
         $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));

tests/Predis/Connection/WebdisConnectionTest.php

@@ -130,7 +130,7 @@ class WebdisConnectionTest extends StandardTestCase
         $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));