fix(auth): selecting database before auth caused error #85
Conversation
|
@jasverix Hi! Thank you for the fix! However, without an explanation what error case this is fixing, I'm left guessing a little bit. The general assumption here is that if an object is passed to the constructor, you can just call |
|
It's a long time since I did this PR, but I am pretty sure it's the Alternative approach is that, in Redis.php, we don't pass |
| if ($auth !== null) { | ||
| $this->driver->auth($auth); | ||
| } | ||
|
|
||
| if ($database !== null) { | ||
| $this->driver->select($database); |
There was a problem hiding this comment.
This causes error if auth is not executed.
| @@ -72,7 +72,7 @@ public static function redis() | |||
| if (is_callable(self::$redisServer)) { | |||
| self::$redis = call_user_func(self::$redisServer, self::$redisDatabase); | |||
| } else { | |||
| self::$redis = new Redis(self::$redisServer, self::$redisDatabase); | |||
| self::$redis = new Redis(self::$redisServer, self::$redisDatabase, null, self::$auth); | |||
There was a problem hiding this comment.
This is passing $redisDatabase, which causes a ->select() in constructor. Line 78 passes auth after the select.
There was a problem hiding this comment.
What's the value of $redisServer that triggers the error for you?
There was a problem hiding this comment.
The value of $redisServer is 10.181.53.75. We are running a local redis server in our network - and that server has auth configured.
There was a problem hiding this comment.
So when $redisServer is set to 10.181.53.75 and $redisDatabase is set to 1, the error occurs. If the $redisDatabase is 0, it does not fail, because ->select() is not ran.
There was a problem hiding this comment.
Why not use tcp://user:pass@host:port/db as a value? The user part of it is ignored, but that wouldn't be part of $auth either so that wouldn't matter
| @@ -72,7 +72,7 @@ public static function redis() | |||
| if (is_callable(self::$redisServer)) { | |||
| self::$redis = call_user_func(self::$redisServer, self::$redisDatabase); | |||
| } else { | |||
| self::$redis = new Redis(self::$redisServer, self::$redisDatabase); | |||
| self::$redis = new Redis(self::$redisServer, self::$redisDatabase, null, self::$auth); | |||
| } | |||
|
|
|||
| if (!empty(self::$auth)) { | |||
There was a problem hiding this comment.
Here, auth is passed, but it is too late.
There was a problem hiding this comment.
Pretty sure the if block after this one should be removed in this PR, since trying to auth twice isn't technically valid.
|
@danhunsaker You merged chrisboulton/php-resque#328 that introduced the |
|
Ultimately, I'd like to rip Credis out and support multiple back ends by making users instantiate their own Redis connection. For now? I'm ok with a rollback. |
danhunsaker
force-pushed
the
fix-auth
branch
2 times, most recently
from
331a316 to
c38c3e7
Compare
No description provided.