Removed NULL assignement for username parameter #1428
Conversation
vladvildanov
added
feature
redis-sentinel
| $parameters['database'] = null; | ||
| $parameters['username'] = null; |
There was a problem hiding this comment.
I recall these changes causing issues in the past, but I can only find #807.
| @@ -51,17 +51,17 @@ public function testParametersForSentinelConnectionShouldUsePasswordForAuthentic | |||
| /** | |||
| * @group disconnected | |||
| */ | |||
| public function testParametersForSentinelConnectionShouldIgnoreDatabaseAndPassword(): void | |||
| public function testParametersForSentinelConnectionShouldIgnoreDatabase(): void | |||
There was a problem hiding this comment.
I think we should merge this into main instead, unless we can keep the existing unit tests unchanged.
There was a problem hiding this comment.
Well, functionality was changed and unit test should be changed according to this. Or you mean that the whole PR should be merged into main?
There was a problem hiding this comment.
What I mean is that people might rely on the removed/changed functionality and if we want to change behavior, we have to either merge into main because it's breaking.
I'm usually being overly precautious and assume people have weird Sentinel setups with varying usersnames for each node, or some other oddities.
If you like we can merge this into 2.x, but we might crash someone's setup and need to revert.
There was a problem hiding this comment.
You're right, it's a breaking change. Here's new PR against main #1429
Since, Redis 6.2 sentinels supports ACL authentication and AUTH command with both username and password. So for now we don't need to override
usernameon Sentinel connectionsCloses #1427