From 41c29bed4ecb09ff83befb1cb9748217b3aeaccc Mon Sep 17 00:00:00 2001 From: Daniele Alessandri Date: Fri, 27 Apr 2012 17:03:32 +0200 Subject: [PATCH] Change how Redis errors such as -ERR replies generates exceptions. The "throw_errors" connection parameter has been removed and replaced by the new "exceptions" client option since exceptions on -ERR replies returned by Redis are not generated by connection classes anymore but are thrown by the client class and other abstractions such as pipeline contexts. This change does not affect much people using the Predis\Client class (aside from the different configuration) but gives much more flexibility to those building their own pieces of code around the internal classes of Predis. --- CHANGELOG.md | 5 ++ lib/Predis/Client.php | 29 ++++++- .../Connection/ComposableStreamConnection.php | 1 - lib/Predis/Connection/PhpiredisConnection.php | 18 +---- lib/Predis/Connection/StreamConnection.php | 9 +-- lib/Predis/Connection/WebdisConnection.php | 13 +--- lib/Predis/ConnectionParameters.php | 2 - lib/Predis/Option/ClientExceptions.php | 36 +++++++++ lib/Predis/Option/ClientOptions.php | 1 + lib/Predis/Pipeline/FireAndForgetExecutor.php | 2 +- lib/Predis/Pipeline/PipelineContext.php | 30 +++++--- .../Pipeline/PipelineExecutorInterface.php | 2 +- lib/Predis/Pipeline/SafeClusterExecutor.php | 6 +- lib/Predis/Pipeline/SafeExecutor.php | 5 +- lib/Predis/Pipeline/StandardExecutor.php | 51 +++++++++---- .../Protocol/Text/ComposableTextProtocol.php | 5 -- .../Protocol/Text/ResponseErrorHandler.php | 8 +- .../Text/ResponseErrorSilentHandler.php | 37 --------- lib/Predis/Protocol/Text/TextProtocol.php | 13 +--- lib/Predis/Transaction/MultiExecContext.php | 9 +++ tests/PHPUnit/ConnectionTestCase.php | 17 +---- tests/Predis/ClientTest.php | 76 +++++++++++++++++++ tests/Predis/ConnectionParametersTest.php | 6 +- tests/Predis/Option/ClientExceptionsTest.php | 31 ++++++++ tests/Predis/Option/ClientOptionsTest.php | 2 + tests/Predis/Pipeline/PipelineContextTest.php | 13 +++- .../Transaction/MultiExecContextTest.php | 53 ++++++++++++- 27 files changed, 320 insertions(+), 160 deletions(-) create mode 100644 lib/Predis/Option/ClientExceptions.php delete mode 100644 lib/Predis/Protocol/Text/ResponseErrorSilentHandler.php create mode 100644 tests/Predis/Option/ClientExceptionsTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e29dc244..55298d2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ v0.8.0 (201x-xx-xx) - `connection_timeout` is now `timeout` - `connection_persistent` is now `persistent` +- The `throw_errors` connection parameter has been removed and replaced by the + new `exceptions` client option since exceptions on -ERR replies returned by + Redis are not generated by connection classes anymore but are thrown by the + client class and other abstractions such as pipeline contexts. + - Cluster and replication connections now extend a new common interface, `Predis\Connection\AggregatedConnectionInterface`. diff --git a/lib/Predis/Client.php b/lib/Predis/Client.php index 1337627e..0fc26f26 100644 --- a/lib/Predis/Client.php +++ b/lib/Predis/Client.php @@ -206,7 +206,13 @@ class Client implements ClientInterface public function __call($method, $arguments) { $command = $this->profile->createCommand($method, $arguments); - return $this->connection->executeCommand($command); + $response = $this->connection->executeCommand($command); + + if ($response instanceof ResponseErrorInterface) { + $this->onResponseError($response); + } + + return $response; } /** @@ -222,7 +228,26 @@ class Client implements ClientInterface */ public function executeCommand(CommandInterface $command) { - return $this->connection->executeCommand($command); + $response = $this->connection->executeCommand($command); + + if ($response instanceof ResponseErrorInterface) { + $this->onResponseError($response); + } + + return $response; + } + + /** + * Handles -ERR responses returned by Redis. + * + * @param ResponseErrorInterface $response The error response instance. + */ + protected function onResponseError(ResponseErrorInterface $response) + { + if ($this->options->exceptions === true) { + $message = $response->getMessage(); + throw new ServerException($message); + } } /** diff --git a/lib/Predis/Connection/ComposableStreamConnection.php b/lib/Predis/Connection/ComposableStreamConnection.php index c0fe9790..ce4d2384 100644 --- a/lib/Predis/Connection/ComposableStreamConnection.php +++ b/lib/Predis/Connection/ComposableStreamConnection.php @@ -42,7 +42,6 @@ class ComposableStreamConnection extends StreamConnection implements ComposableC */ protected function initializeProtocol(ConnectionParametersInterface $parameters) { - $this->protocol->setOption('throw_errors', $parameters->throw_errors); $this->protocol->setOption('iterable_multibulk', $parameters->iterable_multibulk); } diff --git a/lib/Predis/Connection/PhpiredisConnection.php b/lib/Predis/Connection/PhpiredisConnection.php index 21c1983c..52d3ab1c 100644 --- a/lib/Predis/Connection/PhpiredisConnection.php +++ b/lib/Predis/Connection/PhpiredisConnection.php @@ -16,7 +16,6 @@ use Predis\ConnectionParametersInterface; use Predis\ResponseError; use Predis\ResponseQueued; use Predis\ClientException; -use Predis\ServerException; use Predis\NotSupportedException; /** @@ -42,7 +41,6 @@ use Predis\NotSupportedException; * - port: TCP port of the server. * - timeout: timeout to perform the connection. * - read_write_timeout: timeout of read / write operations. - * - throw_errors: -ERR replies treated as exceptions. * * @link http://github.com/seppo0010/phpiredis * @author Daniele Alessandri @@ -104,15 +102,13 @@ class PhpiredisConnection extends AbstractConnection /** * Initializes the protocol reader resource. - * - * @param Boolean $throw_errors Specify if Redis errors throw exceptions. */ - private function initializeReader($throw_errors = true) { + private function initializeReader() { $reader = phpiredis_reader_create(); phpiredis_reader_set_status_handler($reader, $this->getStatusHandler()); - phpiredis_reader_set_error_handler($reader, $this->getErrorHandler($throw_errors)); + phpiredis_reader_set_error_handler($reader, $this->getErrorHandler()); $this->reader = $reader; } @@ -122,7 +118,7 @@ class PhpiredisConnection extends AbstractConnection */ protected function initializeProtocol(ConnectionParametersInterface $parameters) { - $this->initializeReader($parameters->throw_errors); + $this->initializeReader(); } /** @@ -152,14 +148,8 @@ class PhpiredisConnection extends AbstractConnection * @param Boolean $throw_errors Specify if Redis errors throw exceptions. * @return \Closure */ - private function getErrorHandler($throwErrors = true) + private function getErrorHandler() { - if ($throwErrors) { - return function($errorMessage) { - throw new ServerException($errorMessage); - }; - } - return function($errorMessage) { return new ResponseError($errorMessage); }; diff --git a/lib/Predis/Connection/StreamConnection.php b/lib/Predis/Connection/StreamConnection.php index a1ad67db..98d2441e 100644 --- a/lib/Predis/Connection/StreamConnection.php +++ b/lib/Predis/Connection/StreamConnection.php @@ -13,7 +13,6 @@ namespace Predis\Connection; use Predis\ResponseError; use Predis\ResponseQueued; -use Predis\ServerException; use Predis\NotSupportedException; use Predis\ConnectionParametersInterface; use Predis\Command\CommandInterface; @@ -30,7 +29,6 @@ use Predis\Iterator\MultiBulkResponseSimple; * - read_write_timeout: timeout of read / write operations. * - async_connect: performs the connection asynchronously. * - persistent: the connection is left intact after a GC collection. - * - throw_errors: -ERR replies treated as exceptions. * - iterable_multibulk: multibulk replies treated as iterable objects. * * @author Daniele Alessandri @@ -38,7 +36,6 @@ use Predis\Iterator\MultiBulkResponseSimple; class StreamConnection extends AbstractConnection { private $mbiterable; - private $throwErrors; /** * Disconnects from the server and destroys the underlying resource when @@ -57,7 +54,6 @@ class StreamConnection extends AbstractConnection */ protected function initializeProtocol(ConnectionParametersInterface $parameters) { - $this->throwErrors = (bool) $parameters->throw_errors; $this->mbiterable = (bool) $parameters->iterable_multibulk; } @@ -264,9 +260,6 @@ class StreamConnection extends AbstractConnection return (int) $payload; case '-': // error - if ($this->throwErrors) { - throw new ServerException($payload); - } return new ResponseError($payload); default: @@ -301,6 +294,6 @@ class StreamConnection extends AbstractConnection */ public function __sleep() { - return array_merge(parent::__sleep(), array('mbiterable', 'throwErrors')); + return array_merge(parent::__sleep(), array('mbiterable')); } } diff --git a/lib/Predis/Connection/WebdisConnection.php b/lib/Predis/Connection/WebdisConnection.php index a559902b..2e0b1630 100644 --- a/lib/Predis/Connection/WebdisConnection.php +++ b/lib/Predis/Connection/WebdisConnection.php @@ -15,7 +15,6 @@ use Predis\Command\CommandInterface; use Predis\ResponseObjectInterface; use Predis\ConnectionParametersInterface; use Predis\ResponseError; -use Predis\ServerException; use Predis\NotSupportedException; use Predis\Protocol\ProtocolException; use Predis\Connection\ConnectionException; @@ -39,7 +38,6 @@ use Predis\Connection\ConnectionException; * - timeout: timeout to perform the connection. * - user: username for authentication. * - pass: password for authentication. - * - throw_errors: -ERR replies treated as exceptions. * * @link http://webd.is * @link http://github.com/nicolasff/webdis @@ -140,7 +138,7 @@ class WebdisConnection implements SingleConnectionInterface $reader = phpiredis_reader_create(); phpiredis_reader_set_status_handler($reader, $this->getStatusHandler()); - phpiredis_reader_set_error_handler($reader, $this->getErrorHandler($parameters->throw_errors)); + phpiredis_reader_set_error_handler($reader, $this->getErrorHandler()); return $reader; } @@ -160,17 +158,10 @@ class WebdisConnection implements SingleConnectionInterface /** * Gets the handler used by the protocol reader to handle Redis errors. * - * @param Boolean $throwErrors Specify if Redis errors throw exceptions. * @return \Closure */ - protected function getErrorHandler($throwErrors) + protected function getErrorHandler() { - if ($throwErrors) { - return function($errorMessage) { - throw new ServerException($errorMessage); - }; - } - return function($errorMessage) { return new ResponseError($errorMessage); }; diff --git a/lib/Predis/ConnectionParameters.php b/lib/Predis/ConnectionParameters.php index 07a31cf7..730435df 100644 --- a/lib/Predis/ConnectionParameters.php +++ b/lib/Predis/ConnectionParameters.php @@ -28,7 +28,6 @@ class ConnectionParameters implements ConnectionParametersInterface 'port' => 6379, 'timeout' => 5.0, 'iterable_multibulk' => false, - 'throw_errors' => true, ); /** @@ -71,7 +70,6 @@ class ConnectionParameters implements ConnectionParametersInterface 'timeout' => $float, 'read_write_timeout' => $float, 'iterable_multibulk' => $bool, - 'throw_errors' => $bool, ); } diff --git a/lib/Predis/Option/ClientExceptions.php b/lib/Predis/Option/ClientExceptions.php new file mode 100644 index 00000000..ad422dad --- /dev/null +++ b/lib/Predis/Option/ClientExceptions.php @@ -0,0 +1,36 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Predis\Option; + +/** + * Option class used to specify if the client should throw server exceptions. + * + * @author Daniele Alessandri + */ +class ClientExceptions extends AbstractOption +{ + /** + * {@inheritdoc} + */ + public function filter(ClientOptionsInterface $options, $value) + { + return (bool) $value; + } + + /** + * {@inheritdoc} + */ + public function getDefault(ClientOptionsInterface $options) + { + return true; + } +} diff --git a/lib/Predis/Option/ClientOptions.php b/lib/Predis/Option/ClientOptions.php index c04fe66f..f75a11a5 100644 --- a/lib/Predis/Option/ClientOptions.php +++ b/lib/Predis/Option/ClientOptions.php @@ -44,6 +44,7 @@ class ClientOptions implements ClientOptionsInterface 'cluster' => new ClientCluster(), 'replication' => new ClientReplication(), 'prefix' => new ClientPrefix(), + 'exceptions' => new ClientExceptions(), ); } diff --git a/lib/Predis/Pipeline/FireAndForgetExecutor.php b/lib/Predis/Pipeline/FireAndForgetExecutor.php index e5afd903..ce098fcf 100644 --- a/lib/Predis/Pipeline/FireAndForgetExecutor.php +++ b/lib/Predis/Pipeline/FireAndForgetExecutor.php @@ -39,7 +39,7 @@ class FireAndForgetExecutor implements PipelineExecutorInterface /** * {@inheritdoc} */ - public function execute(ConnectionInterface $connection, &$commands) + public function execute(ConnectionInterface $connection, Array &$commands) { $this->checkConnection($connection); diff --git a/lib/Predis/Pipeline/PipelineContext.php b/lib/Predis/Pipeline/PipelineContext.php index 76576726..d97e472e 100644 --- a/lib/Predis/Pipeline/PipelineContext.php +++ b/lib/Predis/Pipeline/PipelineContext.php @@ -30,8 +30,8 @@ class PipelineContext implements BasicClientInterface, ExecutableContextInterfac private $executor; private $pipeline = array(); - private $replies = array(); - private $running = false; + private $replies = array(); + private $running = false; /** * @param ClientInterface Client instance used by the context. @@ -53,18 +53,18 @@ class PipelineContext implements BasicClientInterface, ExecutableContextInterfac */ protected function createExecutor(ClientInterface $client, Array $options) { - if (!$options) { - return new StandardExecutor(); - } - if (isset($options['executor'])) { $executor = $options['executor']; - if (!$executor instanceof PipelineExecutorInterface) { - throw new \InvalidArgumentException( - 'The executor option accepts only instances ' . - 'of Predis\Pipeline\PipelineExecutorInterface' - ); + + if (is_callable($executor)) { + $executor = call_user_func($executor, $client, $options); } + + if (!$executor instanceof PipelineExecutorInterface) { + $message = 'The executor option accepts only instances of Predis\Pipeline\PipelineExecutorInterface'; + throw new \InvalidArgumentException($message); + } + return $executor; } @@ -73,7 +73,11 @@ class PipelineContext implements BasicClientInterface, ExecutableContextInterfac return $isCluster ? new SafeClusterExecutor() : new SafeExecutor(); } - return new StandardExecutor(); + $clientOpts = $client->getOptions(); + $useExceptions = isset($clientOpts->exceptions) ? $clientOpts->exceptions : true; + $executor = new StandardExecutor($useExceptions); + + return $executor; } /** @@ -123,8 +127,10 @@ class PipelineContext implements BasicClientInterface, ExecutableContextInterfac if ($send) { $connection = $this->client->getConnection(); $replies = $this->executor->execute($connection, $this->pipeline); + $this->replies = array_merge($this->replies, $replies); } + $this->pipeline = array(); } diff --git a/lib/Predis/Pipeline/PipelineExecutorInterface.php b/lib/Predis/Pipeline/PipelineExecutorInterface.php index 0b2a518b..3ea994eb 100644 --- a/lib/Predis/Pipeline/PipelineExecutorInterface.php +++ b/lib/Predis/Pipeline/PipelineExecutorInterface.php @@ -28,5 +28,5 @@ interface PipelineExecutorInterface * @param array $commands List of commands. * @return array */ - public function execute(ConnectionInterface $connection, &$commands); + public function execute(ConnectionInterface $connection, Array &$commands); } diff --git a/lib/Predis/Pipeline/SafeClusterExecutor.php b/lib/Predis/Pipeline/SafeClusterExecutor.php index d6ef21bb..0bafc0eb 100644 --- a/lib/Predis/Pipeline/SafeClusterExecutor.php +++ b/lib/Predis/Pipeline/SafeClusterExecutor.php @@ -11,7 +11,6 @@ namespace Predis\Pipeline; -use Predis\ServerException; use Predis\CommunicationException; use Predis\Connection\ConnectionInterface; @@ -27,7 +26,7 @@ class SafeClusterExecutor implements PipelineExecutorInterface /** * {@inheritdoc} */ - public function execute(ConnectionInterface $connection, &$commands) + public function execute(ConnectionInterface $connection, Array &$commands) { $connectionExceptions = array(); $sizeofPipe = count($commands); @@ -64,9 +63,6 @@ class SafeClusterExecutor implements PipelineExecutorInterface $response = $cmdConnection->readResponse($command); $values[] = $response instanceof \Iterator ? iterator_to_array($response) : $response; } - catch (ServerException $exception) { - $values[] = $exception->toResponseError(); - } catch (CommunicationException $exception) { $values[] = $exception; $connectionExceptions[$connectionObjectHash] = $exception; diff --git a/lib/Predis/Pipeline/SafeExecutor.php b/lib/Predis/Pipeline/SafeExecutor.php index e4aa63f7..308f7478 100644 --- a/lib/Predis/Pipeline/SafeExecutor.php +++ b/lib/Predis/Pipeline/SafeExecutor.php @@ -26,7 +26,7 @@ class SafeExecutor implements PipelineExecutorInterface /** * {@inheritdoc} */ - public function execute(ConnectionInterface $connection, &$commands) + public function execute(ConnectionInterface $connection, Array &$commands) { $sizeofPipe = count($commands); $values = array(); @@ -48,9 +48,6 @@ class SafeExecutor implements PipelineExecutorInterface $response = $connection->readResponse($command); $values[] = $response instanceof \Iterator ? iterator_to_array($response) : $response; } - catch (ServerException $exception) { - $values[] = $exception->toResponseError(); - } catch (CommunicationException $exception) { $toAdd = count($commands) - count($values); $values = array_merge($values, array_fill(0, $toAdd, $exception)); diff --git a/lib/Predis/Pipeline/StandardExecutor.php b/lib/Predis/Pipeline/StandardExecutor.php index 0ae6df55..4adf406a 100644 --- a/lib/Predis/Pipeline/StandardExecutor.php +++ b/lib/Predis/Pipeline/StandardExecutor.php @@ -11,9 +11,10 @@ namespace Predis\Pipeline; -use Predis\ServerException; +use Predis\ResponseErrorInterface; use Predis\Connection\ConnectionInterface; use Predis\Connection\ReplicationConnectionInterface; +use Predis\ServerException; /** * Implements the standard pipeline executor strategy used @@ -24,6 +25,14 @@ use Predis\Connection\ReplicationConnectionInterface; */ class StandardExecutor implements PipelineExecutorInterface { + /** + * @param bool $useServerExceptions Specifies if the executor should throw exceptions on server errors. + */ + public function __construct($useServerExceptions = true) + { + $this->useServerExceptions = (bool) $useServerExceptions; + } + /** * Allows the pipeline executor to perform operations on the * connection before starting to execute the commands stored @@ -38,13 +47,29 @@ class StandardExecutor implements PipelineExecutorInterface } } + /** + * Handles -ERR responses returned by Redis. + * + * @param ConnectionInterface $connection The connection that returned the error. + * @param ResponseErrorInterface $response The error response instance. + */ + protected function onResponseError(ConnectionInterface $connection, ResponseErrorInterface $response) + { + // Force disconnection to prevent protocol desynchronization. + $connection->disconnect(); + $message = $response->getMessage(); + + throw new ServerException($message); + } + /** * {@inheritdoc} */ - public function execute(ConnectionInterface $connection, &$commands) + public function execute(ConnectionInterface $connection, Array &$commands) { - $sizeofPipe = count($commands); $values = array(); + $sizeofPipe = count($commands); + $useServerExceptions = $this->useServerExceptions; $this->checkConnection($connection); @@ -52,19 +77,15 @@ class StandardExecutor implements PipelineExecutorInterface $connection->writeCommand($command); } - try { - for ($i = 0; $i < $sizeofPipe; $i++) { - $response = $connection->readResponse($commands[$i]); - $values[] = $response instanceof \Iterator - ? iterator_to_array($response) - : $response; - unset($commands[$i]); + for ($i = 0; $i < $sizeofPipe; $i++) { + $response = $connection->readResponse($commands[$i]); + + if ($response instanceof ResponseErrorInterface && $useServerExceptions === true) { + $this->onResponseError($connection, $response); } - } - catch (ServerException $exception) { - // Force disconnection to prevent protocol desynchronization. - $connection->disconnect(); - throw $exception; + + $values[] = $response instanceof \Iterator ? iterator_to_array($response) : $response; + unset($commands[$i]); } return $values; diff --git a/lib/Predis/Protocol/Text/ComposableTextProtocol.php b/lib/Predis/Protocol/Text/ComposableTextProtocol.php index f185aeee..33c336a2 100644 --- a/lib/Predis/Protocol/Text/ComposableTextProtocol.php +++ b/lib/Predis/Protocol/Text/ComposableTextProtocol.php @@ -66,11 +66,6 @@ class ComposableTextProtocol implements ComposableProtocolInterface $this->reader->setHandler(TextProtocol::PREFIX_MULTI_BULK, $handler); break; - case 'throw_errors': - $handler = $value ? new ResponseErrorHandler() : new ResponseErrorSilentHandler(); - $this->reader->setHandler(TextProtocol::PREFIX_ERROR, $handler); - break; - default: throw new \InvalidArgumentException("The option $option is not supported by the current protocol"); } diff --git a/lib/Predis/Protocol/Text/ResponseErrorHandler.php b/lib/Predis/Protocol/Text/ResponseErrorHandler.php index 3f71c4a8..292dcc6f 100644 --- a/lib/Predis/Protocol/Text/ResponseErrorHandler.php +++ b/lib/Predis/Protocol/Text/ResponseErrorHandler.php @@ -11,7 +11,7 @@ namespace Predis\Protocol\Text; -use Predis\ServerException; +use Predis\ResponseError; use Predis\Protocol\ResponseHandlerInterface; use Predis\Connection\ComposableConnectionInterface; @@ -19,19 +19,19 @@ use Predis\Connection\ComposableConnectionInterface; * Implements a response handler for error replies using the standard wire * protocol defined by Redis. * - * This handler throws an exception to notify the user that an error has + * This handler returns a reply object to notify the user that an error has * occurred on the server. * * @link http://redis.io/topics/protocol * @author Daniele Alessandri */ -class ResponseErrorHandler implements ResponseHandlerInterface +class ResponseErrorSilentHandler implements ResponseHandlerInterface { /** * {@inheritdoc} */ public function handle(ComposableConnectionInterface $connection, $errorMessage) { - throw new ServerException($errorMessage); + return new ResponseError($errorMessage); } } diff --git a/lib/Predis/Protocol/Text/ResponseErrorSilentHandler.php b/lib/Predis/Protocol/Text/ResponseErrorSilentHandler.php deleted file mode 100644 index 292dcc6f..00000000 --- a/lib/Predis/Protocol/Text/ResponseErrorSilentHandler.php +++ /dev/null @@ -1,37 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Predis\Protocol\Text; - -use Predis\ResponseError; -use Predis\Protocol\ResponseHandlerInterface; -use Predis\Connection\ComposableConnectionInterface; - -/** - * Implements a response handler for error replies using the standard wire - * protocol defined by Redis. - * - * This handler returns a reply object to notify the user that an error has - * occurred on the server. - * - * @link http://redis.io/topics/protocol - * @author Daniele Alessandri - */ -class ResponseErrorSilentHandler implements ResponseHandlerInterface -{ - /** - * {@inheritdoc} - */ - public function handle(ComposableConnectionInterface $connection, $errorMessage) - { - return new ResponseError($errorMessage); - } -} diff --git a/lib/Predis/Protocol/Text/TextProtocol.php b/lib/Predis/Protocol/Text/TextProtocol.php index b52aa45e..68464581 100644 --- a/lib/Predis/Protocol/Text/TextProtocol.php +++ b/lib/Predis/Protocol/Text/TextProtocol.php @@ -44,7 +44,6 @@ class TextProtocol implements ProtocolInterface const BUFFER_SIZE = 4096; private $mbiterable; - private $throwErrors; private $serializer; /** @@ -52,9 +51,8 @@ class TextProtocol implements ProtocolInterface */ public function __construct() { - $this->mbiterable = false; - $this->throwErrors = true; - $this->serializer = new TextCommandSerializer(); + $this->mbiterable = false; + $this->serializer = new TextCommandSerializer(); } /** @@ -115,9 +113,6 @@ class TextProtocol implements ProtocolInterface return (int) $payload; case '-': // error - if ($this->throwErrors) { - throw new ServerException($payload); - } return new ResponseError($payload); default: @@ -136,10 +131,6 @@ class TextProtocol implements ProtocolInterface case 'iterable_multibulk': $this->mbiterable = (bool) $value; break; - - case 'throw_errors': - $this->throwErrors = (bool) $value; - break; } } } diff --git a/lib/Predis/Transaction/MultiExecContext.php b/lib/Predis/Transaction/MultiExecContext.php index 0e5924e1..e2583139 100644 --- a/lib/Predis/Transaction/MultiExecContext.php +++ b/lib/Predis/Transaction/MultiExecContext.php @@ -14,6 +14,7 @@ namespace Predis\Transaction; use Predis\ClientInterface; use Predis\BasicClientInterface; use Predis\ExecutableContextInterface; +use Predis\ResponseErrorInterface; use Predis\Command\CommandInterface; use Predis\Helpers; use Predis\ResponseQueued; @@ -379,9 +380,17 @@ class MultiExecContext implements BasicClientInterface, ExecutableContextInterfa $this->onProtocolError("EXEC returned an unexpected number of replies"); } + $clientOpts = $this->client->getOptions(); + $useExceptions = isset($clientOpts->exceptions) ? $clientOpts->exceptions : true; + for ($i = 0; $i < $sizeofReplies; $i++) { $commandReply = $execReply[$i]; + if ($commandReply instanceof ResponseErrorInterface && $useExceptions) { + $message = $commandReply->getMessage(); + throw new ServerException($message); + } + if ($commandReply instanceof \Iterator) { $commandReply = iterator_to_array($commandReply); } diff --git a/tests/PHPUnit/ConnectionTestCase.php b/tests/PHPUnit/ConnectionTestCase.php index 92aad607..03a61829 100644 --- a/tests/PHPUnit/ConnectionTestCase.php +++ b/tests/PHPUnit/ConnectionTestCase.php @@ -263,27 +263,12 @@ abstract class ConnectionTestCase extends StandardTestCase $this->assertSame(3, $connection->read()); } - /** - * @group connected - * @expectedException Predis\ServerException - * @expectedExceptionMessage ERR Operation against a key holding the wrong kind of value - */ - public function testReadsErrorRepliesAsExceptions() - { - $connection = $this->getConnection($profile, true, array('throw_errors' => true)); - - $connection->executeCommand($profile->createCommand('set', array('foo', 'bar'))); - $connection->writeCommand($profile->createCommand('rpush', array('foo', 'baz'))); - - $connection->read(); - } - /** * @group connected */ public function testReadsErrorRepliesAsResponseErrorObjects() { - $connection = $this->getConnection($profile, true, array('throw_errors' => false)); + $connection = $this->getConnection($profile, true); $connection->executeCommand($profile->createCommand('set', array('foo', 'bar'))); $connection->writeCommand($profile->createCommand('rpush', array('foo', 'baz'))); diff --git a/tests/Predis/ClientTest.php b/tests/Predis/ClientTest.php index 6e47d006..c6731d04 100644 --- a/tests/Predis/ClientTest.php +++ b/tests/Predis/ClientTest.php @@ -321,6 +321,44 @@ class ClientTest extends StandardTestCase $this->assertTrue($client->executeCommand($ping)); } + /** + * @group disconnected + * @expectedException Predis\ServerException + * @expectedExceptionMessage ERR Operation against a key holding the wrong kind of value + */ + public function testExecuteCommandThrowsExceptionOnRedisError() + { + $ping = ServerProfile::getDefault()->createCommand('ping', array()); + $expectedResponse = new ResponseError('ERR Operation against a key holding the wrong kind of value'); + + $connection= $this->getMock('Predis\Connection\ConnectionInterface'); + $connection->expects($this->once()) + ->method('executeCommand') + ->will($this->returnValue($expectedResponse)); + + $client = new Client($connection); + $client->executeCommand($ping); + } + + /** + * @group disconnected + */ + public function testExecuteCommandReturnsErrorResponseOnRedisError() + { + $ping = ServerProfile::getDefault()->createCommand('ping', array()); + $expectedResponse = new ResponseError('ERR Operation against a key holding the wrong kind of value'); + + $connection= $this->getMock('Predis\Connection\ConnectionInterface'); + $connection->expects($this->once()) + ->method('executeCommand') + ->will($this->returnValue($expectedResponse)); + + $client = new Client($connection, array('exceptions' => false)); + $response = $client->executeCommand($ping); + + $this->assertSame($response, $expectedResponse); + } + /** * @group disconnected */ @@ -345,6 +383,44 @@ class ClientTest extends StandardTestCase $this->assertTrue($client->ping()); } + /** + * @group disconnected + * @expectedException Predis\ServerException + * @expectedExceptionMessage ERR Operation against a key holding the wrong kind of value + */ + public function testCallingRedisCommandThrowsExceptionOnServerError() + { + $expectedResponse = new ResponseError('ERR Operation against a key holding the wrong kind of value'); + + $connection = $this->getMock('Predis\Connection\ConnectionInterface'); + $connection->expects($this->once()) + ->method('executeCommand') + ->with($this->isInstanceOf('Predis\Command\ConnectionPing')) + ->will($this->returnValue($expectedResponse)); + + $client = new Client($connection); + $client->ping(); + } + + /** + * @group disconnected + */ + public function testCallingRedisCommandReturnsErrorResponseOnRedisError() + { + $expectedResponse = new ResponseError('ERR Operation against a key holding the wrong kind of value'); + + $connection = $this->getMock('Predis\Connection\ConnectionInterface'); + $connection->expects($this->once()) + ->method('executeCommand') + ->with($this->isInstanceOf('Predis\Command\ConnectionPing')) + ->will($this->returnValue($expectedResponse)); + + $client = new Client($connection, array('exceptions' => false)); + $response = $client->ping(); + + $this->assertSame($response, $expectedResponse); + } + /** * @group disconnected * @expectedException Predis\ClientException diff --git a/tests/Predis/ConnectionParametersTest.php b/tests/Predis/ConnectionParametersTest.php index 4ce1dd0c..d9e9de01 100644 --- a/tests/Predis/ConnectionParametersTest.php +++ b/tests/Predis/ConnectionParametersTest.php @@ -30,7 +30,6 @@ class ConnectionParametersTest extends StandardTestCase $this->assertEquals($defaults['scheme'], $parameters->scheme); $this->assertEquals($defaults['host'], $parameters->host); $this->assertEquals($defaults['port'], $parameters->port); - $this->assertEquals($defaults['throw_errors'], $parameters->throw_errors); $this->assertEquals($defaults['iterable_multibulk'], $parameters->iterable_multibulk); $this->assertEquals($defaults['timeout'], $parameters->timeout); } @@ -76,7 +75,7 @@ class ConnectionParametersTest extends StandardTestCase $overrides = array( 'port' => 7000, 'database' => 5, - 'throw_errors' => false, + 'iterable_multibulk' => false, 'custom' => 'foobar', ); @@ -87,7 +86,7 @@ class ConnectionParametersTest extends StandardTestCase $this->assertEquals($overrides['port'], $parameters->port); $this->assertEquals($overrides['database'], $parameters->database); - $this->assertEquals($overrides['throw_errors'], $parameters->throw_errors); + $this->assertEquals($overrides['iterable_multibulk'], $parameters->iterable_multibulk); $this->assertTrue(isset($parameters->custom)); $this->assertEquals($overrides['custom'], $parameters->custom); @@ -142,7 +141,6 @@ class ConnectionParametersTest extends StandardTestCase 'port' => 6379, 'timeout' => 5.0, 'iterable_multibulk' => false, - 'throw_errors' => true, ); } diff --git a/tests/Predis/Option/ClientExceptionsTest.php b/tests/Predis/Option/ClientExceptionsTest.php new file mode 100644 index 00000000..27fe298c --- /dev/null +++ b/tests/Predis/Option/ClientExceptionsTest.php @@ -0,0 +1,31 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Predis\Option; + +use \PHPUnit_Framework_TestCase as StandardTestCase; + +/** + * + */ +class ClientExceptionsTest extends StandardTestCase +{ + /** + * @group disconnected + */ + public function testDefaultReturnsTrue() + { + $options = $this->getMock('Predis\Option\ClientOptionsInterface'); + $option = new ClientExceptions(); + + $this->assertTrue($option->getDefault($options)); + } +} diff --git a/tests/Predis/Option/ClientOptionsTest.php b/tests/Predis/Option/ClientOptionsTest.php index 812c67cd..17bdfad8 100644 --- a/tests/Predis/Option/ClientOptionsTest.php +++ b/tests/Predis/Option/ClientOptionsTest.php @@ -42,12 +42,14 @@ class ClientOptionsTest extends StandardTestCase 'connections' => 'Predis\ConnectionFactory', 'prefix' => 'prefix:', 'profile' => '2.0', + 'exceptions' => false, )); $this->assertInstanceOf('Predis\ConnectionFactoryInterface', $options->connections); $this->assertInstanceOf('Predis\Profile\ServerProfileInterface', $options->profile); $this->assertInstanceOf('Predis\Connection\ClusterConnectionInterface', $options->cluster); $this->assertInstanceOf('Predis\Command\Processor\CommandProcessorInterface', $options->prefix); + $this->assertInternalType('bool', $options->exceptions); } /** diff --git a/tests/Predis/Pipeline/PipelineContextTest.php b/tests/Predis/Pipeline/PipelineContextTest.php index 6940b12d..39d68b7c 100644 --- a/tests/Predis/Pipeline/PipelineContextTest.php +++ b/tests/Predis/Pipeline/PipelineContextTest.php @@ -378,7 +378,7 @@ class PipelineContextTest extends StandardTestCase */ public function testIntegrationWithServerErrorInCallableBlock() { - $client = $this->getClient(array('throw_errors' => false)); + $client = $this->getClient(array(), array('exceptions' => false)); $results = $client->pipeline(function($pipe) { $pipe->set('foo', 'bar'); @@ -400,9 +400,10 @@ class PipelineContextTest extends StandardTestCase * server instance to perform integration tests. * * @return array Additional connection parameters. - * @return Client client instance. + * @return array Additional client options. + * @return Client New client instance. */ - protected function getClient(Array $parameters = array()) + protected function getClient(Array $parameters = array(), Array $options = array()) { $parameters = array_merge(array( 'scheme' => 'tcp', @@ -411,7 +412,11 @@ class PipelineContextTest extends StandardTestCase 'database' => REDIS_SERVER_DBNUM, ), $parameters); - $client = new Client($parameters, array('profile' => REDIS_SERVER_VERSION)); + $options = array_merge(array( + 'profile' => REDIS_SERVER_VERSION, + ), $options); + + $client = new Client($parameters, $options); $client->connect(); $client->flushdb(); diff --git a/tests/Predis/Transaction/MultiExecContextTest.php b/tests/Predis/Transaction/MultiExecContextTest.php index cde2d6e1..2a6e6f4a 100644 --- a/tests/Predis/Transaction/MultiExecContextTest.php +++ b/tests/Predis/Transaction/MultiExecContextTest.php @@ -494,6 +494,48 @@ class MultiExecContextTest extends StandardTestCase $this->assertFalse($client->exists('foo')); } + /** + * @group connected + */ + public function testIntegrationThrowsExceptionOnRedisErrorInBlock() + { + $client = $this->getClient(); + $exception = null; + $value = (string) rand(); + + try { + $client->multiExec(function($tx) use($value) { + $tx->set('foo', 'bar'); + $tx->lpush('foo', 'bar'); + $tx->set('foo', $value); + }); + } + catch (ServerException $ex) { + $exception = $ex; + } + + $this->assertInstanceOf('Predis\ResponseErrorInterface', $exception); + $this->assertSame($value, $client->get('foo')); + } + + /** + * @group connected + */ + public function testIntegrationReturnsErrorObjectOnRedisErrorInBlock() + { + $client = $this->getClient(array(), array('exceptions' => false)); + + $replies = $client->multiExec(function($tx) { + $tx->set('foo', 'bar'); + $tx->lpush('foo', 'bar'); + $tx->echo('foobar'); + }); + + $this->assertTrue($replies[0]); + $this->assertInstanceOf('Predis\ResponseErrorInterface', $replies[1]); + $this->assertSame('foobar', $replies[2]); + } + /** * @group connected */ @@ -704,10 +746,11 @@ class MultiExecContextTest extends StandardTestCase * Returns a client instance connected to the specified Redis * server instance to perform integration tests. * - * @return array Additional connection parameters. + * @param array Additional connection parameters. + * @param array Additional client options. * @return Client client instance. */ - protected function getClient(Array $parameters = array()) + protected function getClient(Array $parameters = array(), Array $options = array()) { $parameters = array_merge(array( 'scheme' => 'tcp', @@ -716,7 +759,11 @@ class MultiExecContextTest extends StandardTestCase 'database' => REDIS_SERVER_DBNUM, ), $parameters); - $client = new Client($parameters, array('profile' => REDIS_SERVER_VERSION)); + $options = array_merge(array( + 'profile' => REDIS_SERVER_VERSION + ), $options); + + $client = new Client($parameters, $options); $client->connect(); $client->flushdb();