Remove getRedisIdentifier from Redis interfaces. This is not the task of our library but the task of the underlying libraries.

This also fixes that some of the Predis ConnectionInterface objects don't have a __toString() method.

Author: willemstuursma

classes/mutex/PHPRedisMutex.php

@@ -48,9 +48,8 @@ protected function add($redisAPI, string $key, string $value, int $expire): bool
             return $redisAPI->set($key, $value, ["nx", "ex" => $expire]);
         } catch (RedisException $e) {
             $message = sprintf(
-                "Failed to acquire lock for key '%s' at %s",
-                $key,
-                $this->getRedisIdentifier($redisAPI)
+                "Failed to acquire lock for key '%s'",
+                $key
             );
             throw new LockAcquireException($message, 0, $e);
         }
@@ -73,20 +72,7 @@ protected function evalScript($redis, string $script, int $numkeys, array $argum
         try {
             return $redis->eval($script, $arguments, $numkeys);
         } catch (RedisException $e) {
-            $message = sprintf(
-                "Failed to release lock at %s",
-                $this->getRedisIdentifier($redis)
-            );
-            throw new LockReleaseException($message, 0, $e);
+            throw new LockReleaseException("Failed to release lock", 0, $e);
         }
     }
-
-    /**
-     * @internal
-     */
-    protected function getRedisIdentifier($redis)
-    {
-        /** @var Redis $redis */
-        return sprintf("redis://%s:%d?database=%s", $redis->getHost(), $redis->getPort(), $redis->getDBNum());
-    }
 }

classes/mutex/PredisMutex.php

@@ -43,42 +43,23 @@ protected function add($redisAPI, string $key, string $value, int $expire): bool
             return $redisAPI->set($key, $value, "EX", $expire, "NX") !== null;
         } catch (PredisException $e) {
             $message = sprintf(
-                "Failed to acquire lock for key '%s' at %s",
-                $key,
-                $this->getRedisIdentifier($redisAPI)
+                "Failed to acquire lock for key '%s'",
+                $key
             );
             throw new LockAcquireException($message, 0, $e);
         }
     }
 
+    /**
+     * @throws LockReleaseException
+     */
     protected function evalScript($client, string $script, int $numkeys, array $arguments)
     {
         /** @var ClientInterface $client */
         try {
             return $client->eval($script, $numkeys, ...$arguments);
         } catch (PredisException $e) {
-            $message = sprintf(
-                "Failed to release lock at %s",
-                $this->getRedisIdentifier($client)
-            );
-            throw new LockReleaseException($message, 0, $e);
+            throw new LockReleaseException("Failed to release lock", 0, $e);
         }
     }
-
-    /**
-     * @internal
-     */
-    protected function getRedisIdentifier($client)
-    {
-        /** @var ClientInterface $client */
-
-        /*
-         * Note that there is no __toString() on the \Predis\Connection\ConnectionInterface
-         * class. Some classes such as \Predis\Connection\Aggregate\SentinelReplication don't
-         * have a __toString() method.
-         *
-         * todo fix this
-         */
-        return (string) $client->getConnection();
-    }
 }

classes/mutex/RedisMutex.php

@@ -77,7 +77,7 @@ protected function acquire(string $key, int $expire): bool
         $errored  = 0;
         $this->token = \random_bytes(16);
         $exception   = null;
-        foreach ($this->redisAPIs as $redis) {
+        foreach ($this->redisAPIs as $index => $redis) {
             try {
                 if ($this->add($redis, $key, $this->token, $expire)) {
                     $acquired++;
@@ -87,10 +87,9 @@ protected function acquire(string $key, int $expire): bool
                 $context = [
                     "key"       => $key,
                     "token"     => $this->token,
-                    "redis"     => $this->getRedisIdentifier($redis),
                     "exception" => $exception
                 ];
-                $this->logger->warning("Could not set {key} = {token} at {redis}.", $context);
+                $this->logger->warning("Could not set {key} = {token} at server #{index}.", $context);
 
                 $errored++;
             }
@@ -146,10 +145,9 @@ protected function release(string $key): bool
                 $context = [
                     "key"       => $key,
                     "token"     => $this->token,
-                    "redis"     => $this->getRedisIdentifier($redis),
                     "exception" => $e
                 ];
-                $this->logger->warning("Could not unset {key} = {token} at {redis}.", $context);
+                $this->logger->warning("Could not unset {key} = {token} at server #{index}.", $context);
             }
         }
         return $this->isMajority($released);
@@ -188,13 +186,4 @@ abstract protected function add($redisAPI, string $key, string $value, int $expi
      * @throws LockReleaseException An unexpected error happened.
      */
     abstract protected function evalScript($redisAPI, string $script, int $numkeys, array $arguments);
-    
-    /**
-     * Returns a string representation of the Redis API.
-     *
-     * @param mixed  $redisAPI The connected Redis API.
-     * @return string The identifier.
-     * @internal
-     */
-    abstract protected function getRedisIdentifier($redisAPI);
 }

tests/mutex/PredisMutexTest.php

@@ -6,6 +6,7 @@
 use PHPUnit\Framework\TestCase;
 use Predis\ClientInterface;
 use Predis\PredisException;
+use Psr\Log\LoggerInterface;
 
 /**
  * Tests for PredisMutex.
@@ -27,6 +28,11 @@ class PredisMutexTest extends TestCase
      */
     private $mutex;
 
+    /**
+     * @var LoggerInterface|MockObject
+     */
+    private $logger;
+
     protected function setUp()
     {
         parent::setUp();
@@ -36,6 +42,9 @@ protected function setUp()
             ->getMock();
 
         $this->mutex = new PredisMutex([$this->client], "test");
+
+        $this->logger = $this->createMock(LoggerInterface::class);
+        $this->mutex->setLogger($this->logger);
     }
 
     /**
@@ -50,6 +59,9 @@ public function testAddFailsToSetKey()
             ->with("lock_test", $this->isType("string"), "EX", 4, "NX")
             ->willReturn(null);
 
+        $this->logger->expects($this->never())
+            ->method("warning");
+
         $this->mutex->synchronized(
             function () {
                 $this->fail("Code execution is not expected");
@@ -69,6 +81,10 @@ public function testAddErrors()
             ->with("lock_test", $this->isType("string"), "EX", 4, "NX")
             ->willThrowException($this->createMock(PredisException::class));
 
+        $this->logger->expects($this->once())
+            ->method("warning")
+            ->with("Could not set {key} = {token} at server #{index}.", $this->anything());
+
         $this->mutex->synchronized(
             function () {
                 $this->fail("Code execution is not expected");
@@ -114,6 +130,10 @@ public function testEvalScriptFails()
             ->with($this->anything(), 1, "lock_test", $this->isType("string"))
             ->willThrowException($this->createMock(PredisException::class));
 
+        $this->logger->expects($this->once())
+            ->method("warning")
+            ->with("Could not unset {key} = {token} at server #{index}.", $this->anything());
+
         $executed = false;
 
         $this->mutex->synchronized(function () use (&$executed): void {