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

@@ -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)]);
     }
 
     /**

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;
+    }
+
 }

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;

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();

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();
 

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
      */