Skip to content
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

[redis] Authentication support added #350

Closed
wants to merge 1 commit into from
Closed

[redis] Authentication support added #350

wants to merge 1 commit into from

Conversation

FrankGiesecke
Copy link
Contributor

Use the authentication information provided by the config after parsing a dsn.

Closes #349

if (array_key_exists('auth', $this->config)) {
$this->redis->auth($this->config['auth']);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we need the first if if (array_key_exists('pass', $this->config)) {.

<?php
            if (array_key_exists('pass', $this->config)) {
                $this->redis->auth($this->config['pass']);
            }

Copy link
Contributor Author

@FrankGiesecke FrankGiesecke Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for services like Heroku.
They provide a token for authentication in the password part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code I posted does exactly the same but with one if statement.

@makasim makasim changed the title Authentication support added [redis] Authentication support added Feb 5, 2018
@makasim
Copy link
Member

makasim commented Feb 15, 2018

This will be possible once #372 is merged. Like this:

<?php
use Enqueue\Redis\RedisConnectionFactory;
use Enqueue\Redis\PRedis;
 
$config = []; 
$options = [];

$predis = new \PRedis\Client($config, $options);
$predis->auth('aPassword');
$redis = new PRedis();

$factory = new RedisConnectionFactory(['vendor' => 'custom', 'redis' => $redis]);

@FrankGiesecke
Copy link
Contributor Author

@makasim Does it mean, that authentication will only be supported by the PRedis client?

@makasim
Copy link
Member

makasim commented Feb 19, 2018

Yes, the Enqueue\PhpRedis constructor has to be adjusted to accept the instance of \Redis class.

@FrankGiesecke
Copy link
Contributor Author

Please reconsider your decision. It would be very useful to consume the authentication from within the DSN. Creating an own instance of a redis client only to support authentication is much more overhead and not necessary.

@MaSpeng
Copy link

MaSpeng commented Apr 3, 2018

I completely agree, when using various large hosters an authorization to the redis Instance is required. Only for this purpose a pre-configured redis instance must be explicitly created instead of drawing the maximum benefit from the existing functionalities, especially since the adaptation effort is apparently very low and non-invasive.

@dgoujard
Copy link
Contributor

dgoujard commented Jun 2, 2018

I agree too, it's not so uncommon to use redis with password, it's not really practical to create an custom vendor. It's more easy with DSN, other transporters handle password mechanism in DSN

@makasim
Copy link
Member

makasim commented Jun 4, 2018

I don't mind adding authentication option if it is widely used and many want to see it supported.

@FrankGiesecke
Copy link
Contributor Author

@makasim I'm glad to hear this and I would be appreciated if you wold merge my changes.

@makasim makasim reopened this Jun 4, 2018
@makasim
Copy link
Member

makasim commented Jun 4, 2018

Can you add auth support to predis too?

@makasim
Copy link
Member

makasim commented Jun 4, 2018

and some tests.

@makasim
Copy link
Member

makasim commented Jun 4, 2018

@makasim
Copy link
Member

makasim commented Aug 13, 2018

merged as part of #497. both ways are supported

you can pass pass or an instance of Enqueue\Redis.

@makasim makasim closed this Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants