From f06a41cfdcef4ae7d3ccbe59022b2a15f428e4e5 Mon Sep 17 00:00:00 2001 From: Daniele Alessandri Date: Fri, 21 Aug 2020 13:30:23 +0200 Subject: [PATCH] Change method to undefine commands in factory. The previous implementation was not good because we were exposing in the public API an internal implementation detail of the base factory class, furthermore it made Predis\Command\Factory::define() confusing. Having a separate method to undefine commands in the factory is self-explanatory. We also changed `Predis\Configuration\Option\Commands` accordingly when a dictionary of $commandID => $classCommand is passed to the "commands" client option and $classCommand is NULL. A few minor changes (mostly cosmetic or documentation) were applied too. From feedback to PR #644. --- src/Command/Factory.php | 44 +++++++++++++------ src/Command/RedisFactory.php | 14 ++++++ src/Configuration/Option/Commands.php | 8 +++- tests/Predis/ClientTest.php | 2 +- tests/Predis/Command/RedisFactoryTest.php | 8 ++-- .../Configuration/Option/CommandsTest.php | 23 ++++++++++ 6 files changed, 78 insertions(+), 21 deletions(-) diff --git a/src/Command/Factory.php b/src/Command/Factory.php index c569389d..671d997c 100644 --- a/src/Command/Factory.php +++ b/src/Command/Factory.php @@ -50,11 +50,11 @@ abstract class Factory implements FactoryInterface } /** - * Returns the FQN of a class that represents the specified command ID. + * Returns the FQCN of a class that represents the specified command ID. * * @codeCoverageIgnore * - * @param string $commandID Command ID. + * @param string $commandID Command ID * * @return string|null */ @@ -73,7 +73,7 @@ abstract class Factory implements FactoryInterface if (!$commandClass = $this->getCommandClass($commandID)) { $commandID = strtoupper($commandID); - throw new ClientException("Command '$commandID' is not a registered Redis command."); + throw new ClientException("Command `$commandID` is not a registered Redis command."); } $command = new $commandClass(); @@ -87,24 +87,40 @@ abstract class Factory implements FactoryInterface } /** - * Defines a new command in the factory. + * Defines a command in the factory. * - * @param string $commandID Command ID. - * @param string $class Fully-qualified name of a Predis\Command\CommandInterface. + * Only classes implementing Predis\Command\CommandInterface are allowed to + * handle a command. If the command specified by its ID is already handled + * by the factory, the underlying command class is replaced by the new one. + * + * @param string $commandID Command ID + * @param string $commandClass FQCN of a class implementing Predis\Command\CommandInterface * * @throws \InvalidArgumentException */ - public function defineCommand($commandID, $class) + public function defineCommand($commandID, $commandClass) { - if ($class !== null) { - $reflection = new \ReflectionClass($class); - - if (!$reflection->isSubclassOf('Predis\Command\CommandInterface')) { - throw new \InvalidArgumentException("The class '$class' is not a valid command class."); - } + if (!is_a($commandClass, 'Predis\Command\CommandInterface', true)) { + throw new \InvalidArgumentException( + "Class $commandClass must implement Predis\Command\CommandInterface" + ); } - $this->commands[strtoupper($commandID)] = $class; + $this->commands[strtoupper($commandID)] = $commandClass; + } + + /** + * Undefines a command in the factory. + * + * When the factory already has a class handler associated to the specified + * command ID it is removed from the map of known commands. Nothing happens + * when the command is not handled by the factory. + * + * @param string $commandID Command ID + */ + public function undefineCommand($commandID) + { + unset($this->commands[strtoupper($commandID)]); } /** diff --git a/src/Command/RedisFactory.php b/src/Command/RedisFactory.php index 75e38431..fdfcd702 100644 --- a/src/Command/RedisFactory.php +++ b/src/Command/RedisFactory.php @@ -47,4 +47,18 @@ class RedisFactory extends Factory return $commandClass; } + + /** + * {@inheritdoc} + */ + public function undefineCommand($commandID) + { + // NOTE: we explicitly associate `NULL` to the command ID in the map + // instead of the parent's `unset()` because our subclass tries to load + // a predefined class from the Predis\Command\Redis namespace when no + // explicit mapping is defined, see RedisFactory::getCommandClass() for + // details of the implementation of this mechanism. + $this->commands[strtoupper($commandID)] = null; + } + } diff --git a/src/Configuration/Option/Commands.php b/src/Configuration/Option/Commands.php index 94af9e54..e520f2c5 100644 --- a/src/Configuration/Option/Commands.php +++ b/src/Configuration/Option/Commands.php @@ -35,8 +35,12 @@ class Commands implements OptionInterface if (is_array($value)) { $commands = $this->getDefault($options); - foreach ($value as $commandID => $classFQN) { - $commands->defineCommand($commandID, $classFQN); + foreach ($value as $commandID => $commandClass) { + if ($commandClass === null) { + $commands->undefineCommand($commandID); + } else { + $commands->defineCommand($commandID, $commandClass); + } } return $commands; diff --git a/tests/Predis/ClientTest.php b/tests/Predis/ClientTest.php index d2f657a3..c1842d3c 100644 --- a/tests/Predis/ClientTest.php +++ b/tests/Predis/ClientTest.php @@ -702,7 +702,7 @@ class ClientTest extends PredisTestCase public function testThrowsExceptionOnNonRegisteredRedisCommand() { $this->expectException('Predis\ClientException'); - $this->expectExceptionMessage("Command 'INVALIDCOMMAND' is not a registered Redis command"); + $this->expectExceptionMessage("Command `INVALIDCOMMAND` is not a registered Redis command"); $client = new Client(); $client->invalidCommand(); diff --git a/tests/Predis/Command/RedisFactoryTest.php b/tests/Predis/Command/RedisFactoryTest.php index db3f9641..11c49940 100644 --- a/tests/Predis/Command/RedisFactoryTest.php +++ b/tests/Predis/Command/RedisFactoryTest.php @@ -102,7 +102,7 @@ class RedisFactoryTest extends PredisTestCase $this->assertTrue($factory->supportsCommand('PING')); $this->assertSame('Predis\Command\Redis\PING', $factory->getCommandClass('PING')); - $factory->defineCommand('PING', null); + $factory->undefineCommand('PING'); $this->assertFalse($factory->supportsCommand('PING')); $this->assertNull($factory->getCommandClass('PING')); @@ -121,7 +121,7 @@ class RedisFactoryTest extends PredisTestCase $this->assertTrue($factory->supportsCommand('MOCK')); $this->assertSame($commandClass, $factory->getCommandClass('MOCK')); - $factory->defineCommand('MOCK', null); + $factory->undefineCommand('MOCK'); $this->assertFalse($factory->supportsCommand('MOCK')); $this->assertNull($factory->getCommandClass('MOCK')); @@ -133,7 +133,7 @@ class RedisFactoryTest extends PredisTestCase public function testDefineInvalidCommand() { $this->expectException('InvalidArgumentException'); - $this->expectExceptionMessage("The class 'stdClass' is not a valid command class."); + $this->expectExceptionMessage("Class stdClass must implement Predis\Command\CommandInterface"); $factory = new RedisFactory(); @@ -175,7 +175,7 @@ class RedisFactoryTest extends PredisTestCase public function testCreateUndefinedCommand() { $this->expectException('Predis\ClientException'); - $this->expectExceptionMessage("Command 'UNKNOWN' is not a registered Redis command."); + $this->expectExceptionMessage("Command `UNKNOWN` is not a registered Redis command."); $factory = new RedisFactory(); diff --git a/tests/Predis/Configuration/Option/CommandsTest.php b/tests/Predis/Configuration/Option/CommandsTest.php index 17c7f3f0..a329b983 100644 --- a/tests/Predis/Configuration/Option/CommandsTest.php +++ b/tests/Predis/Configuration/Option/CommandsTest.php @@ -101,6 +101,29 @@ class CommandsTest extends PredisTestCase $this->assertSame('Predis\Command\RawCommand', $commands->getCommandClass('BAR')); } + /** + * @group disconnected + */ + public function testAcceptsDictionaryOfCommandsWithNullsToUndefineCommandsAsValue() + { + $option = new Commands(); + + $options = $this->getMockBuilder('Predis\Configuration\OptionsInterface')->getMock(); + + $input = array( + 'ECHO' => null, + 'EVAL' => null, + 'FOO' => null, + ); + + $commands = $option->filter($options, $input); + + $this->assertInstanceOf('Predis\Command\FactoryInterface', $commands); + $this->assertNull($commands->getCommandClass('ECHO')); + $this->assertNull($commands->getCommandClass('EVAL')); + $this->assertNull($commands->getCommandClass('FOO')); + } + /** * @group disconnected */