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 Session Handler - Multiple Host DSN #34477

Closed
BeyerJC opened this issue Nov 21, 2019 · 3 comments
Closed

Redis Session Handler - Multiple Host DSN #34477

BeyerJC opened this issue Nov 21, 2019 · 3 comments
Labels
Bug Help wanted Issues and PRs which are looking for volunteers to complete them. HttpFoundation Status: Needs Review

Comments

@BeyerJC
Copy link

BeyerJC commented Nov 21, 2019

Hello everyone!

#34177 says we can now use DSNs for configuring the session handler. For me, it works fine when using a single Redis server. But what if i try to use multiple servers ? I copied a example dsn from the cache component which looks like this :

'redis:?host[localhost]&host[localhost:6379]&host[/var/run/redis.sock:]&auth=my-password&redis_cluster=1'

or in the yaml

handler_id: 'redis:?host[localhost]&host[localhost:6379]&host[/var/run/redis.sock:]&auth=my-password&redis_cluster=1'

This causes an error because "redis:?" is not proccessed as a redis dsn.

Is my configuration wrong or is this a bug in the implementation ?

Thanks in advance !

@nicolas-grekas
Copy link
Member

Looks like something we forgot about (we check for redis:// and not only for redis: isn't it?)

PR welcome to fix the issue (the patch attached to #34177 should be a good start to know where the code needs to be adjusted.)

@nicolas-grekas nicolas-grekas added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Nov 24, 2019
@BeyerJC
Copy link
Author

BeyerJC commented Nov 24, 2019

Yes, thats what i thought when looking into the class. Should be easily fixed, i will test that tomorrow and create a PR.

nicolas-grekas added a commit that referenced this issue Dec 13, 2019
…Jan Christoph Beyer)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpFoundation] fix redis multi host dsn not recognized

| Q             | A
| ------------- | ---
| Branch?       |  4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34477
| License       | MIT

#34177 added support configurating session handlers with DSNs. It was no possible to pass a redis-DSN like
`redis:?host[localhost]&host[localhost:6379]&host[/var/run/redis.sock:]&auth=my-password&redis_cluster=1'`

since the check was
`case 0 === strpos($connection, 'redis://'):`

Commits
-------

81ba07a fix redis multi host dsn not recognized
@stajnert
Copy link

@WhiteRabbitDE @nicolas-grekas The same should be done in https://raw.githubusercontent.com/symfony/lock/master/Store/StoreFactory.php for lock component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Help wanted Issues and PRs which are looking for volunteers to complete them. HttpFoundation Status: Needs Review
Projects
None yet
Development

No branches or pull requests

5 participants