-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fix redis:// being rejected and username:password passing invalid auth to Redis #42
Conversation
Fix last commit not changing the length of the string in expected response
src/Factory.php
Outdated
// password@ | ||
else if (isset($parts['user'])) { | ||
$parts['auth'] = $parts['user']; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fix the coding style here? E.g. } else if (...) {
and fix the indentation.
ping @daneren2005 :) |
Thanks for this PR and your patience! 👍 This library will need to receive a minor breaking change in order to replace the deprecated SocketClient with the new Socket component and I'll look into addressing outstanding PRs in the next days! Stay tuned :) |
I fixed the indentation |
@clue replacing deprecated SocketClient with Socket component are great news for me, i'll be waiting anxious for this (it will be nice connect to redis through a twemproxy socket). Please tell me if you need some modest help. |
$parts['auth'] = $parts['pass']; | ||
} elseif (isset($parts['user'])) { | ||
// password@ | ||
$parts['auth'] = $parts['user']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to properly parse the URI, I suppose it makes sense to only support the "pass" component and ignore the "user" component entirely, so this block should probably be removed? 👍
We should also make sure to update the documentation and/or tests accordingly 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it checks for user as the auth is that the parser is parsing the pass in pass@host as the user. I added a test for both cases and it would be a backwards incompatible change to remove support for pass@host. It doesn't seem like it adds anything to remove support for that.
Thanks for the help guys! The failing tests are caused by an incompatibility with PHPUnit which should be added to Any help / PRs would be much appreciated 👍 |
I would rather you just pick it up since I am not able to at the moment. |
As discussed in #40, Sending auth
username:password
to a Redis server doesn't make any sense right now. Some services such as Redis Cloud automatically setup the urls in the form ofbogus_username:password@server
and only the password part should be sent on to Redis. Alsoredis://
is a valid schema prefix.