bug symfony#59348 [Lock] Fix predis command error checking (dciprian-petrisor)

This PR was merged into the 7.2 branch.

Discussion
----------

[Lock] Fix predis command error checking

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix symfony#59087
| License       | MIT

We seem to have had an incompatibility with how Predis clients are initialized (`'exceptions' => false`) and the implementation of RedisStore, which only surfaced now due to the recent EVAL -> EVALSHA changes.

According to https://github.com/symfony/symfony/blob/7.2/src/Symfony/Component/Cache/Traits/RedisTrait.php#L65 and
https://github.com/symfony/symfony/blob/7.2/src/Symfony/Component/Cache/Traits/RedisTrait.php#L419 , the Predis client is always initialized with exceptions disabled; it returns Error objects instead.

This PR fixes the tests to replicate this behaviour and the implementation.

An small additional change was made regarding error checking, to ensure the initial `evalSha` does not fail with anything else besides the expected `NOSCRIPT` error.

Commits
-------

4816402 Fix predis command error checking

Author: fabpot

src/Symfony/Component/Lock/Store/RedisStore.php

@@ -11,7 +11,7 @@
 
 namespace Symfony\Component\Lock\Store;
 
-use Predis\Response\ServerException;
+use Predis\Response\Error;
 use Relay\Relay;
 use Symfony\Component\Lock\Exception\InvalidTtlException;
 use Symfony\Component\Lock\Exception\LockConflictedException;
@@ -250,10 +250,10 @@ private function evaluate(string $script, string $resource, array $args): mixed
                 }
 
                 $result = $this->redis->evalSha($scriptSha, array_merge([$resource], $args), 1);
+            }
 
-                if (null !== $err = $this->redis->getLastError()) {
-                    throw new LockStorageException($err);
-                }
+            if (null !== $err = $this->redis->getLastError()) {
+                throw new LockStorageException($err);
             }
 
             return $result;
@@ -273,37 +273,32 @@ private function evaluate(string $script, string $resource, array $args): mixed
                 }
 
                 $result = $client->evalSha($scriptSha, array_merge([$resource], $args), 1);
+            }
 
-                if (null !== $err = $client->getLastError()) {
-                    throw new LockStorageException($err);
-                }
+            if (null !== $err = $client->getLastError()) {
+                throw new LockStorageException($err);
             }
 
             return $result;
         }
 
         \assert($this->redis instanceof \Predis\ClientInterface);
 
-        try {
-            return $this->redis->evalSha($scriptSha, 1, $resource, ...$args);
-        } catch (ServerException $e) {
-            // Fallthrough only if we need to load the script
-            if (self::NO_SCRIPT_ERROR_MESSAGE !== $e->getMessage()) {
-                throw new LockStorageException($e->getMessage(), $e->getCode(), $e);
+        $result = $this->redis->evalSha($scriptSha, 1, $resource, ...$args);
+        if ($result instanceof Error && self::NO_SCRIPT_ERROR_MESSAGE === $result->getMessage()) {
+            $result = $this->redis->script('LOAD', $script);
+            if ($result instanceof Error) {
+                throw new LockStorageException($result->getMessage());
             }
-        }
 
-        try {
-            $this->redis->script('LOAD', $script);
-        } catch (ServerException $e) {
-            throw new LockStorageException($e->getMessage(), $e->getCode(), $e);
+            $result = $this->redis->evalSha($scriptSha, 1, $resource, ...$args);
         }
 
-        try {
-            return $this->redis->evalSha($scriptSha, 1, $resource, ...$args);
-        } catch (ServerException $e) {
-            throw new LockStorageException($e->getMessage(), $e->getCode(), $e);
+        if ($result instanceof Error) {
+            throw new LockStorageException($result->getMessage());
         }
+
+        return $result;
     }
 
     private function getUniqueToken(Key $key): string

src/Symfony/Component/Lock/Tests/Store/CombinedStoreTest.php

@@ -39,7 +39,10 @@ protected function getClockDelay(): int
 
     public function getStore(): PersistingStoreInterface
     {
-        $redis = new \Predis\Client(array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => 6379]));
+        $redis = new \Predis\Client(
+            array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => 6379]),
+            ['exceptions' => false],
+        );
 
         try {
             $redis->connect();

src/Symfony/Component/Lock/Tests/Store/PredisStoreTest.php

@@ -20,7 +20,10 @@ class PredisStoreTest extends AbstractRedisStoreTestCase
 {
     public static function setUpBeforeClass(): void
     {
-        $redis = new \Predis\Client(array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null]));
+        $redis = new \Predis\Client(
+            array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null]),
+            ['exceptions' => false],
+        );
         try {
             $redis->connect();
         } catch (\Exception $e) {
@@ -30,7 +33,10 @@ public static function setUpBeforeClass(): void
 
     protected function getRedisConnection(): \Predis\Client
     {
-        $redis = new \Predis\Client(array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null]));
+        $redis = new \Predis\Client(
+            array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null]),
+            ['exceptions' => false],
+        );
         $redis->connect();
 
         return $redis;