Redis "parseConnectionConfigWithUrl" breaks clusters

[Huggyduggy]

• Laravel Version: 5.8.20
• PHP Version: 7.3.6
• Database Driver & Version: phpredis 4.3.0

Disclaimer:

I sure hope I didn't miss any configuration change, I did my best at digging into it.

Description:

Starting with this commit c88de24 , redis configuration is parsed from URL, too. Afterwards, "driver" and "username" keys are removed from the array.

When using redis in cluster mode with phpredis, configuration looks as following (worked fine until updating today):

'redis' => [
'client' => 'phpredis',
'options' => [
'cluster' => 'redis',
],
'clusters' => [
'default' => [
[
'scheme' => env('REDIS_SCHEME', 'tcp'),
'host' => env('REDIS_HOST', 'localhost'),
'password' => env('REDIS_PASSWORD', null),
'port' => env('REDIS_PORT', 6379),
'database' => env('REDIS_DATABASE', 0),
],
],
],
],

Note that the default cluster is an array with a length of 1 (non-assoc. array, index 0). Therefore, when RedisManager calls parseConnectionConfiguration(), it receives [0 => array], rather than the config-array itself - with only 1 key (0)

God knows why, but in_array(0, ['driver', 'username']) yields true. The issue can be fixed by applying strict type checks: in_array($key, ['driver', 'username'], true). As a result, the [0 => array] $config array becomes empty ([]), and a "Must pass seeds" --> "Couldn't map cluster keyspace using any provided seed" is thrown.

As I haven't yet contributed to laravel (or any other open-source project) and I really don't want to mess with your time, I thought describing a possible fix might be quicker. I have not checked this possible solutions for side-effects.

[driesvints]

Afaik Laravel 7.20 isn't out yet 😅

Which one are you using?

[Huggyduggy]

5.8.20 was released 1 hour ago (https://github.com/laravel/framework/releases), @driesvints . It first occured when updating a few hours ago to 5.8.19 (from 5.8.17, which worked fine)

Edit: Oh boy, by bad. Spent quite some time today updating some internal servers from PHP 7.2 to 7.3.x , got me confused :D

[driesvints]

Ping @mathieutu

[mathieutu]

Thanks @driesvints, I'll look into it as soon as possible.

[mathieutu]

Indeed you're right @Huggyduggy!

It's a mistake from me, I didn't see that clusters was a non-assoc array of assoc arrays (php all-in-one arrays.. sic 🙄).

I can fix that in a next PR, BUT:

@driesvints if we had this bug, it's because the Redis manager isn't tested at all, and so we wasn't able to detect easily this regression. AND this manager can't be properly tested because we can't mock the connectors, as the connector method instantiate directly the adapters (adapters without any common interface btw):

 protected function connector()
    {
        switch ($this->driver) {
            case 'predis':
                return new Connectors\PredisConnector;
            case 'phpredis':
                return new Connectors\PhpRedisConnector;
        }
    }

So:

• I can just fix the bug as I think it, and someone (@Huggyduggy?) will have to check it on his side to validate. Quick but dirty and not sustainable.
• We could also test properly this class but with significant modifications, so you (or/and the boss) should approve that before, if you want to invest on this part of the fw.

Thx,
Matt'

[Huggyduggy]

Seems like @taylorotwell fixed it with 5.8.21 (2bcb405) - works fine here. I'll close the issue, thanks for fixing!

[mathieutu]

Ok, so it will be the quick a dirty way! 🤷‍♂️

[driesvints]

@mathieutu maybe we can do this for the next release. Feel free to send in a pr to the master branch if you want :)