Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Cache] Send Predis SSL options in the $hosts parameter#50074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you confirm that this change would still work?
diff --git a/src/Symfony/Component/Cache/Tests/Adapter/PredisAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/PredisAdapterTest.phpindex e326dba76b..a35859d2b6 100644--- a/src/Symfony/Component/Cache/Tests/Adapter/PredisAdapterTest.php+++ b/src/Symfony/Component/Cache/Tests/Adapter/PredisAdapterTest.php@@ -48,4 +48,29 @@ class PredisAdapterTest extends AbstractRedisAdapterTestCase ]; $this->assertSame($params, $connection->getParameters()->toArray()); }++ public function testCreateSslConnection()+ {+ $redisHost = getenv('REDIS_HOST');++ $redis = RedisAdapter::createConnection('rediss://'.$redisHost.'/1?ssl[verify_peer]=0', ['class' => \Predis\Client::class, 'timeout' => 3]);+ $this->assertInstanceOf(\Predis\Client::class, $redis);++ $connection = $redis->getConnection();+ $this->assertInstanceOf(StreamConnection::class, $connection);++ $redisHost = explode(':', $redisHost);+ $params = [+ 'scheme' => 'tls',+ 'host' => $redisHost[0],+ 'port' => (int) ($redisHost[1] ?? 6379),+ 'ssl' => ['verify_peer' => '0'],+ 'persistent' => 0,+ 'timeout' => 3,+ 'read_write_timeout' => 0,+ 'tcp_nodelay' => true,+ 'database' => '1',+ ];+ $this->assertSame($params, $connection->getParameters()->toArray());+ } }diff --git a/src/Symfony/Component/Cache/Traits/RedisTrait.php b/src/Symfony/Component/Cache/Traits/RedisTrait.phpindex 07633a9d3f..67d8663169 100644--- a/src/Symfony/Component/Cache/Traits/RedisTrait.php+++ b/src/Symfony/Component/Cache/Traits/RedisTrait.php@@ -349,7 +349,7 @@ trait RedisTrait } $params['exceptions'] = false;- $redis = new $class($hosts, array_diff_key($params, array_diff_key(self::$defaultConnectionOptions, ['ssl' => null])));+ $redis = new $class($hosts, array_diff_key($params, self::$defaultConnectionOptions)); if (isset($params['redis_sentinel'])) { $redis->getConnection()->setSentinelTimeout($params['timeout']); }
dff3752 tocd7cd2fCompare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I applied my patch, please report back if there's an issue with it.
nicolas-grekas commentedApr 21, 2023
Thank you@magnusnordlander. |
Predis accepts SSL options in the
$hostsparameter, not in the$paramsparameter. From my perspective, this is really only applicable when usingrediss://host:portstyle DSNs, where you might want to add?ssl[verify_peer]=0or something similar.I'm unsure how to write a good test for this, since there doesn't seem to be any standard Redis host with TLS that requires additional options in the test runner. Happy to take suggestions on how to approach a test, if that's deemed necessary.