Plenty of Redis servers now use authentication. Here's a patch to add support to both drivers.

Marking this "major" because this client doesn't support a substantial set of Redis servers right now.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David Strauss’s picture

FileSize
3.33 KB

Here's an updated patch that also adds the password to the admin screen.

joshk’s picture

+1 for this! ;)

pounard’s picture

Assigned: David Strauss » pounard
Status: Active » Needs review

Yes indeed, nice feature, I will review it and probably commit it as soon as I have some spare time (I will try within the week). Thanks.

David Strauss’s picture

If you're looking for a place to focus testing, I would try the Predis back-end with passwords. I've only really tested the native PhpRedis changes.

pounard’s picture

Status: Needs review » Fixed

Reviewed your patch, just changed "PASSWORD" occurences to "PASS" only (I now am regretting this I should have named it AUTH).

Pushed and a new 7.x-2.0-alpha5 is en route and will arrive in a few minutes.

Thanks!

David Strauss’s picture

No, it shouldn't be called "auth." The command is "AUTH," but the argument is named "password." The value is being passed in as the password argument, so the variable should be called "password." Additionally, variables ought to be nouns (like "password"), not verbs (like "auth").

Here are the docs on the command:
http://redis.io/commands/auth

pounard’s picture

Yup, I did read this page, guess you are right. Still, this is "pass" now, and if you feel it wrong, I'd be happy to revert it, but right now I'm too lazy to do it.

David Strauss’s picture

Sorry to be difficult, but I would prefer "password." Normally, I wouldn't care between "pass" and "password," but we'll need to make a configuration change if the variable is "pass." Thanks.

pounard’s picture

Status: Fixed » Needs work

Okay, I will change it back, just not right now it's sunday and I need some rest, but maybe in the week.

populist’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

Here is a patch to make the change in question.

pounard’s picture

Commited. A new release will come up in the day, thanks to both of you!

pounard’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit 6e738ce on 7.x-2.x, 7.x-2.x-sharding, 7.x-2.x-path by pounard:
    #1439398 - authored by David Strauss - Add support for password...
  • Commit f7ea7e9 on 7.x-2.x, 7.x-2.x-sharding, 7.x-2.x-path by pounard:
    Fixed #1439398 follow up - authored by populist - original patch by...
Marko B’s picture

Issue summary: View changes

Why is this not in version 7.3?

Marko B’s picture

Version: 7.x-2.0-alpha4 » 7.x-3.11
Status: Closed (fixed) » Needs review
Marko B’s picture

Status: Needs review » Needs work

  • pounard committed 6e738ce on 8.x-1.x
    #1439398 - authored by David Strauss - Add support for password...
  • pounard committed f7ea7e9 on 8.x-1.x
    Fixed #1439398 follow up - authored by populist - original patch by...
tunic’s picture

Status: Needs work » Fixed

It seems code to support password is already in module source code (both 7.x and 8.x).

C-Logemann’s picture

Version: 7.x-3.11 » 8.x-1.x-dev
Assigned: pounard » Unassigned

@tunic Yes, I can confirm this for D9. So I was wondering about this issue update in my mail inbox.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.