Fix predis replication using true as config value for predis (#693)

Co-authored-by: Gabriel Ostrolucký <[email protected]> Fixes #692
snc · Jan 8, 2023 · aee156d · aee156d
1 parent 8e2b190
commit aee156d
Show file tree

Hide file tree

Showing 11 changed files with 92 additions and 13 deletions.
diff --git a/composer.json b/composer.json
@@ -22,6 +22,7 @@
     ],
     "require": {
         "php": "^7.4 || ^8.0",
+        "symfony/deprecation-contracts": "^2 || ^3",
         "symfony/framework-bundle": "^4.4 || ^5.3 || ^6.0",
         "symfony/http-foundation": "^4.4 || ^5.3 || ^6.0",
         "symfony/service-contracts": ">=1.0",

diff --git a/docs/README.md b/docs/README.md
@@ -94,14 +94,14 @@ snc_redis:
             type: predis
             alias: default
             dsn:
-                - redis://master-host?alias=master
+                - redis://master-host?role=master
                 - redis://slave-host1
                 - redis://slave-host2
             options:
-                replication: true
+                replication: predis
 ```
 
-Please note that the master dsn connection needs to be tagged with the ```master``` alias.
+Please note that the master dsn connection needs to be tagged with the ```master``` role.
 If not, `predis` will complain.
 
 A setup using `predis`, `phpredis` or `relay` sentinel replication could look like this:

diff --git a/src/DependencyInjection/Configuration/Configuration.php b/src/DependencyInjection/Configuration/Configuration.php
@@ -30,6 +30,7 @@
 
 use function class_exists;
 use function is_iterable;
+use function trigger_deprecation;
 
 class Configuration implements ConfigurationInterface
 {
@@ -127,7 +128,21 @@ private function addClientsSection(ArrayNodeDefinition $rootNode): void
                                     ->scalarNode('serialization')->defaultValue('default')->end()
                                     ->scalarNode('cluster')->defaultNull()->end()
                                     ->scalarNode('prefix')->defaultNull()->end()
-                                    ->enumNode('replication')->values([true, 'sentinel'])->end()
+                                    ->enumNode('replication')
+                                        ->values([true, 'predis', 'sentinel'])
+                                        ->beforeNormalization()
+                                            ->ifTrue(static fn ($v) => $v === true)
+                                            ->then(static function () {
+                                                trigger_deprecation(
+                                                    'snc/redis-bundle',
+                                                    '4.6',
+                                                    'Setting true for "clients.options.replication" is deprecated. Use "predis" or "sentinel" instead',
+                                                );
+
+                                                return 'predis';
+                                            })
+                                        ->end()
+                                    ->end()
                                     ->scalarNode('service')->defaultNull()->end()
                                     ->enumNode('slave_failover')->values(['none', 'error', 'distribute', 'distribute_slaves'])->end()
                                     ->arrayNode('parameters')

diff --git a/src/DependencyInjection/Configuration/RedisDsn.php b/src/DependencyInjection/Configuration/RedisDsn.php
@@ -50,6 +50,8 @@ class RedisDsn
 
     protected ?string $alias = null;
 
+    protected ?string $role = null;
+
     public function __construct(string $dsn)
     {
         $this->dsn = $dsn;
@@ -107,6 +109,11 @@ public function getAlias(): ?string
         return $this->alias;
     }
 
+    public function getRole(): ?string
+    {
+        return $this->role;
+    }
+
     public function getPersistentId(): string
     {
         return md5($this->dsn);
@@ -191,6 +198,10 @@ protected function parseParameters(array $matches): string
             $this->alias = $params['alias'];
         }
 
+        if (!empty($params['role'])) {
+            $this->role = $params['role'];
+        }
+
         return '';
     }
 

diff --git a/src/Factory/PredisParametersFactory.php b/src/Factory/PredisParametersFactory.php
@@ -63,6 +63,10 @@ private static function parseDsn(RedisDsn $dsn): array
             $options['alias'] = $dsn->getAlias();
         }
 
+        if ($dsn->getRole() !== null) {
+            $options['role'] = $dsn->getRole();
+        }
+
         return array_filter($options, static fn ($value) => $value !== null);
     }
 }
diff --git a/tests/DependencyInjection/SncRedisExtensionTest.php b/tests/DependencyInjection/SncRedisExtensionTest.php
@@ -277,11 +277,11 @@ public function testClientReplicationOption(): void
         $extension->load([$config], $container = $this->getContainer());
 
         $options = $container->getDefinition('snc_redis.client.default_options')->getArgument(0);
-        $this->assertTrue($options['replication']);
+        $this->assertSame('predis', $options['replication']);
         $parameters = $container->getDefinition('snc_redis.default')->getArgument(0);
         $this->assertEquals('snc_redis.connection.master_parameters.default', (string) $parameters[0]);
         $masterParameters = $container->getDefinition((string) $parameters[0])->getArgument(0);
-        $this->assertTrue($masterParameters['replication']);
+        $this->assertSame('predis', $masterParameters['replication']);
 
         $this->assertIsArray($container->findTaggedServiceIds('snc_redis.client'));
         $this->assertEquals(['snc_redis.default' => [['alias' => 'default']]], $container->findTaggedServiceIds('snc_redis.client'));
@@ -606,7 +606,7 @@ private function getReplicationYamlConfig(): string
             - redis://localhost?alias=master
             - redis://otherhost
         options:
-            replication: true
+            replication: predis
 EOF;
     }
 
@@ -672,7 +672,7 @@ private function getMultipleReplicationYamlConfig(): string
             - redis://defaulthost?alias=master
             - redis://defaultslave
         options:
-            replication: true
+            replication: predis
             prefix: defaultprefix
     second:
         type: predis
@@ -681,7 +681,7 @@ private function getMultipleReplicationYamlConfig(): string
             - redis://secondmaster?alias=master
             - redis://secondslave
         options:
-            replication: true
+            replication: sentinel
             prefix: secondprefix
 EOF;
     }

diff --git a/tests/Factory/PredisParametersFactoryTest.php b/tests/Factory/PredisParametersFactoryTest.php
@@ -84,12 +84,12 @@ public function createDp(): array
             [
                 'redis://localhost?alias=master',
                 Parameters::class,
-                ['replication' => true],
+                ['replication' => 'predis'],
                 [
                     'scheme' => 'tcp',
                     'host' => 'localhost',
                     'port' => 6379,
-                    'replication' => true,
+                    'replication' => 'predis',
                     'password' => null,
                     'weight' => null,
                     'alias' => 'master',
@@ -100,14 +100,14 @@ public function createDp(): array
                 'redis://localhost?alias=connection_alias',
                 Parameters::class,
                 [
-                    'replication' => true,
+                    'replication' => 'predis',
                     'alias' => 'client_alias',
                 ],
                 [
                     'scheme' => 'tcp',
                     'host' => 'localhost',
                     'port' => 6379,
-                    'replication' => true,
+                    'replication' => 'predis',
                     'password' => null,
                     'weight' => null,
                     'alias' => 'connection_alias',

diff --git a/tests/Functional/App/Controller/PredisReplication.php b/tests/Functional/App/Controller/PredisReplication.php
@@ -0,0 +1,28 @@
+<?php
+
+/*
+ * This file is part of the SncRedisBundle package.
+ *
+ * (c) Henrik Westphal <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Snc\RedisBundle\Tests\Functional\App\Controller;
+
+use Predis\Client;
+use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
+use Symfony\Component\HttpFoundation\JsonResponse;
+
+final class PredisReplication extends AbstractController
+{
+    public function __invoke(Client $predisReplication): JsonResponse
+    {
+        $predisReplication->set('foo', 'bar');
+
+        return new JsonResponse(['result' => $predisReplication->get('foo')]);
+    }
+}
diff --git a/tests/Functional/App/Kernel.php b/tests/Functional/App/Kernel.php
@@ -16,6 +16,7 @@
 use ReflectionObject;
 use Snc\RedisBundle\SncRedisBundle;
 use Snc\RedisBundle\Tests\Functional\App\Controller\Controller;
+use Snc\RedisBundle\Tests\Functional\App\Controller\PredisReplication;
 use Symfony\Bundle\FrameworkBundle\FrameworkBundle;
 use Symfony\Bundle\TwigBundle\TwigBundle;
 use Symfony\Bundle\WebProfilerBundle\WebProfilerBundle;
@@ -76,6 +77,7 @@ public function loadRoutes(LoaderInterface $loader): RouteCollection
 
         $collection = new RouteCollection();
         $collection->add('home', new Route('/', ['_controller' => Controller::class]));
+        $collection->add('predis_replication', new Route('/predis_replication', ['_controller' => PredisReplication::class]));
 
         return $collection;
     }

diff --git a/tests/Functional/App/config.yaml b/tests/Functional/App/config.yaml
@@ -26,6 +26,14 @@ snc_redis:
             alias: cache
             dsn: redis://sncredis@localhost/1
             logging: false
+        predis_replication:
+            type: predis
+            alias: predis_replication
+            dsn:
+                - redis://sncredis@localhost/1?role=master
+                - redis://sncredis@localhost/2
+            options:
+                replication: true
         cluster:
             type: predis
             alias: cluster
@@ -62,6 +70,8 @@ services:
         autowire: true
         autoconfigure: true
         public: false
+        bind:
+            Predis\Client $predisReplication: '@snc_redis.predis_replication'
 
     Redis: '@snc_redis.default'
 

diff --git a/tests/Functional/IntegrationTest.php b/tests/Functional/IntegrationTest.php
@@ -77,6 +77,14 @@ public function testIntegration(): void
         $this->assertSame(0, $redisLogger->getNbCommands());
     }
 
+    /** @group legacy */
+    public function testPredisReplication(): void
+    {
+        $this->client->request('GET', '/predis_replication');
+
+        $this->assertSame(Response::HTTP_OK, $this->client->getResponse()->getStatusCode());
+    }
+
     private function profileRequest(string $method, string $uri): Response
     {
         $client = $this->client;