Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cache] Issue with RedisTrait::createConnection miss processing redis_cluster value #50509

Closed
darkanakin41 opened this issue May 31, 2023 · 6 comments

Comments

@darkanakin41
Copy link
Contributor

Symfony version(s) affected

6.3.0

Description

When trying to initialize a redis cluster connection through symfony/cache RedisAdapter::createConnection, the method returns a RedisArray instead of a RedisCluster

With this issue, i have to manually set the class in the DSN to \RedisCluster in order for it to work.

How to reproduce

Simply pass one of the following strings as DSN to the method :

RedisAdapter::createConnection('redis:?host[redis_cluster:7000]&host[redis_cluster:7001]&redis_cluster=1&timeout=1&read_timeout=2&persistent=1')

RedisAdapter::createConnection('redis:?host[redis_cluster:7000]&host[redis_cluster:7001]&redis_cluster=true&timeout=1&read_timeout=2&persistent=1')

You will get a RedisArray class.

Possible Solution

In the RedisTrait class, starting from line 180 :

$class = $params['class'] ?? match (true) {
            (bool) $params['redis_cluster'] => \extension_loaded('redis') ? \RedisCluster::class : \Predis\Client::class,
            isset($params['redis_sentinel']) => match (true) {
                \extension_loaded('redis') => \Redis::class,
                \extension_loaded('relay') => Relay::class,
                default => \Predis\Client::class,
            },
            1 < \count($hosts) && \extension_loaded('redis') => 1 < \count($hosts) ? \RedisArray::class : \Redis::class,
            \extension_loaded('redis') => \Redis::class,
            \extension_loaded('relay') => Relay::class,
            default => \Predis\Client::class,
        };

Additional Context

No response

@xabbuh
Copy link
Member

xabbuh commented Jun 5, 2023

Would you mind sending a pull request with what you have in mind?

@lugosium
Copy link

lugosium commented Jun 5, 2023

Hello,

After upgrading to Symfony 6.3 i have issue with the container, redis and the lint:container:

First array member is not a valid class name or object

In \Symfony\Component\DependencyInjection\ContainerBuilder::createService:1120

$service = $factory(...$arguments);

$factory:

^ array:2 [
  0 => null
  1 => "createConnection"
]

$arguments:

^ array:2 [
  0 => "redis://lugosium-redis"
  1 => array:1 [
    "lazy" => true
  ]

It does not seems to correctly determine the Redis adapter.
I am not using Redis cluster option.

Hope it helps.

@pkruithof
Copy link
Contributor

We just got bit by this: we had some downtime because the application could not connect to our Redis cluster. Although it's a good opportunity for us to start thinking about smoke tests again, I wonder why Symfony 6.3 was shipped knowing about this bug. I understand that you cannot delay such a release because of some bug report, but maybe the change in this class could have been reverted before the 6.3 release?

I'm curious about the process here, and if this is something to review. To be clear: I fully understand these things can happen, but it decreases the trust with upgrading a minor release a bit.

@stof
Copy link
Member

stof commented Jun 6, 2023

@pkruithof this bug was reported after the release

@pkruithof
Copy link
Contributor

Ah crap, I should've checked that, I was thinking it was released about 3/4 days ago. Sorry about that.

darkanakin41 added a commit to darkanakin41/symfony that referenced this issue Jun 7, 2023
When the dsn contains `redis_cluster=1` or `redis_cluster=true` the value was not properly parsed, and the class generated wan not the expected RedisCluster
@darkanakin41
Copy link
Contributor Author

PR opened :)

nicolas-grekas added a commit that referenced this issue Jun 8, 2023
…anakin41)

This PR was submitted for the 6.3 branch but it was squashed and merged into the 5.4 branch instead.

Discussion
----------

[Cache] Fix RedisTrait::createConnection for cluster

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | No
| Deprecations? | No
| Tickets       | Fix #50509
| License       | MIT
| Doc PR        | N/A

When the dsn contains `redis_cluster=1` or `redis_cluster=true` the value was not properly parsed, and the class generated was not the expected RedisCluster.

The reason is that the PHP core function `match` make a `===` check.

So, as the dsn is a string, the `$params['redis_cluster']` needs to be force to a bool

Commits
-------

ca8e328 [Cache] Fix RedisTrait::createConnection for cluster
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants