#428 #356 Fix support for DSN env variable with phpredis

[B-Galati]

fix #428 #356

Basically the client is always instantiated at runtime thanks to PhpredisClientFactory class.

[B-Galati]

Hello @fpondepeyre @Ensaerel @tgalopin @romain-pierre,

I would be glad if you can try it out on your setup :-)

[fpondepeyre]

sorry but I don't know how to checkout this PR in my project... @B-Galati

[ghost]

You can add:

"repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/B-Galati/SncRedisBundle.git"
        }
    ]

And target to the branch as "snc/redis-bundle": "dev-fix-428-356", then, do composer update snc/redis-bundle

[ghost]

@B-Galati LGTM, but I can't confirm that the placeholder is correctly handled after the cache is built. Because in my environment, the vars are available and stay the same before and after the cache build.

[fpondepeyre]

it's seem work perfectly, no errors...

[B-Galati]

Thanks for the feedback @fpondepeyre @romain-pierre

[B-Galati]

Waiting for symfony/symfony#27470 to be released.

[B-Galati]

Once released the CI should be green.

[curry684]

I'll merge this when Symfony 4.1.1 is out and look into the other PRs.

[B-Galati]

The CI looks good now :D

[jhonnynho]

This changes are available in 2.x-dev?
Because I have the error setting the envs vars on snc_redis.yml on SYmfony 4.1.1

[curry684]

Both on 2.x-dev and 2.1.4 yes. What error are you seeing?

[jhonnynho]

I get this error

php_network_getaddresses: getaddrinfo failed: Name does not resolve [tcp://:6379]

I Have this config on my snc_redis.yml

snc_redis:
    clients:
        default:
            type: predis
            alias: default
            dsn:"redis://%env(REDIS_HOST)%:%env(REDIS_PORT)%/%env(REDIS_DBIDX)%"
            logging: "%kernel.debug%"

Symfony 4.1.1

[jhonnynho]

I dont get that error updating to 3.x-dev, but I don't know if exists a major changes between 2.1.x-dev and 3.x-dev

In the 2.1.x-dev version the error dissapear if only add in the dns param = %env(REDIS_HOST)% , only that.

That have sense for you? @curry684

[B-Galati]

@jhonnynho @curry684 I was able to reproduce the issue. It happens when using a specific a path within the DSN, e.g. /%env(REDIS_DBIDX)% for the given example. It looks like this "path" should match a database index.

I also noticed that if the path is numeric, everything is working as expected but otherwise it's not. It seems to be a normal behavior considering the fact that database index can only be an integer. Please let me know if I am wrong.

@jhonnynho Can you give us an example of what you are using as values ?

It looks like it was working in 2.1.3 but I think the value was just removed by the predis parser (we are not using that method anymore). https://github.com/nrk/predis/blob/111d100ee389d624036b46b35ed0c9ac59c71313/src/Connection/Parameters.php#L123

As per 2.1.4, both phpredis and predis DSN are parsed by RedisDsn class.

I guess we could improve DX by throwing any kind of error message when such a case is happening (e.g. string in path instead of an integer)

[curry684]

Nice work @B-Galati, thanks for all your efforts 😄

If this is indeed the case we should just treat it as a DX issue and improve the error, agreed.

[curry684]

Btw I didn't merge the 2.1.4 fixes into master yet so it's correct that 3.x-dev still has the old (wrong) behavior.

[jhonnynho]

Hi @B-Galati and @curry684

Here an example of the values that I have in my env

REDIS_HOST=redis
REDIS_PORT=6379
REDIS_DBIDX=tokens

To the DBIDX could be a string value, not necessary a integer, you can use both on Redis.

For now I'm use the version 2.1.3 on my project, works as expected.

I'll be waiting some news about this. Best.

[B-Galati]

@jhonnynho Thank you. I just tried with a 3.2 instance and got the following error.

root@49ac88ca7322:/data# redis-cli
127.0.0.1:6379> select 0
OK
127.0.0.1:6379> select tokens
(error) ERR invalid DB index

It looks like the value should be between 0 and 15.

The documentation is not really clear about this: https://redis.io/commands/select. So I am not 100% sure.

[jhonnynho]

@B-Galati, well, in my case I use a LIST on redis. For me works ok, I don't get errors adding a string like a list name.

On Redis CLI I put "KEYS *" and I can see my list.

[B-Galati]

@jhonnynho Sorry, I meant that REDIS_DBIDX value is used by the SELECT command under the wood in order to select the database index that the connection should use.

I am not sure what you meant with LIST command.
I do think that before that PR, your REDIS_DBIDX value was either : removed, replaced by 0 or replaced by 1.
So it looked like it was working but it was not really working as expected.

Please let me know If I am misunderstanding something.

[jhonnynho]

@B-Galati, well, let's see, in my case, I use Redis to set in a List all the tokens generated on the system. So I run the "lpush" and "lrange" to work with that list. I understand Redis do a SELECT to select the database. I'm not defining a redis database, maybe Redis create one per default and inside that it creates the list that I use. I supose.

[B-Galati]

Exactly that. The default database index is 0.

[curry684]

The documentation is not really clear about this

It is?

Select the Redis logical database having the specified zero-based numeric index. New connections always use the database 0.

It's numeric, so tokens is invalid and must throw.

Also refer to http://www.rediscookbook.org/multiple_databases.html:

In Redis, databases are identified by an integer index, not by a database name. By default, a client is connected to database 0. With the SELECT command you can switch to a different database:

redis> select 3
OK

All subsequent commands will then use database 3, until you issue another SELECT.

[B-Galati]

My bad, thank you @curry684 👍