From aee156d728ffddbc86ac4a8f83b174d2cdd7b4b7 Mon Sep 17 00:00:00 2001 From: Fran Moreno Date: Sun, 8 Jan 2023 19:40:54 +0100 Subject: [PATCH] Fix predis replication using true as config value for predis (#693) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Gabriel Ostrolucký Fixes https://github.com/snc/SncRedisBundle/issues/692 --- composer.json | 1 + docs/README.md | 6 ++-- .../Configuration/Configuration.php | 17 ++++++++++- .../Configuration/RedisDsn.php | 11 ++++++++ src/Factory/PredisParametersFactory.php | 4 +++ .../SncRedisExtensionTest.php | 10 +++---- tests/Factory/PredisParametersFactoryTest.php | 8 +++--- .../App/Controller/PredisReplication.php | 28 +++++++++++++++++++ tests/Functional/App/Kernel.php | 2 ++ tests/Functional/App/config.yaml | 10 +++++++ tests/Functional/IntegrationTest.php | 8 ++++++ 11 files changed, 92 insertions(+), 13 deletions(-) create mode 100644 tests/Functional/App/Controller/PredisReplication.php diff --git a/composer.json b/composer.json index 4a4ee767..f37dfd5c 100644 --- 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 index fb4d8198..dd204e0b 100644 --- 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 index 739db3a4..27ed16d6 100644 --- 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 index b7ebb66f..86d05792 100644 --- 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 index 6884324e..8724b664 100644 --- 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 index be8ad137..5cf352d9 100644 --- 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 index 8f6cb4b9..54754316 100644 --- 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 new file mode 100644 index 00000000..807d4c00 --- /dev/null +++ b/tests/Functional/App/Controller/PredisReplication.php @@ -0,0 +1,28 @@ + + * + * 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 index c50ae233..bab09110 100644 --- 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 index f72ebfda..48bfb400 100644 --- 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 index 1fc501a4..3b187b81 100644 --- 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;