NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Cache] Fixed password used to make the redis connection.#20026

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.1fromErikSaunier:ticket_20025

Sep 23, 2016

Merged

[Cache] Fixed password used to make the redis connection.#20026

fabpot merged 1 commit intosymfony:3.1fromErikSaunier:ticket_20025

Sep 23, 2016

Conversation

Copy link

Contributor

ErikSaunier commentedSep 22, 2016•
edited
Loading

Q	A
Branch?	3.1
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20025
License	MIT
Doc PR	http://symfony.com/blog/new-in-symfony-3-1-cache-component

I do not know if it's possible to provide a test asREDIS_HOST is provided by Travis inRedisAdapterTest.php.

carsonbot added Status: Needs Review Cache Bug labels

Sep 22, 2016

ErikSaunier changed the title~~[Cache] Fixed password used to make the redis connection.~~[Cache] WIP Fixed password used to make the redis connection.

Sep 22, 2016

ErikSaunier changed the title~~[Cache] WIP Fixed password used to make the redis connection.~~[Cache] Fixed password used to make the redis connection.

Sep 22, 2016

Copy link

Member

nicolas-grekas commentedSep 22, 2016•
edited
Loading

Redis doesn't have user:password concepts, it only has a single "secret" one, seehttp://redis.io/commands/AUTH . What if this secret contains a ":"? What are you fixing that doesn't work today?

Copy link

ContributorAuthor

ErikSaunier commentedSep 23, 2016

I'm using Heroku and there Redis add on. The url provided in the connection settings looks like this :redis://e:password@ec2-23-217-18-82.eu-west-1.compute.amazonaws.com:21039

As Heroku provide such url it would be nice to support it or report a tricks in the Symfony documentation.

As an exemple, a trick already exist insnc/SncRedisBundle RedisDsn.php for this case.

nicolas-grekas reviewed

Sep 23, 2016

View reviewed changes

src/Symfony/Component/Cache/Adapter/RedisAdapter.php Outdated

		$params =preg_replace_callback('#^redis://(?:([^@]*)@)?#',function ($m)use (&$auth) {
		if (isset($m[1])) {
		$auth =$m[1];
		$auth =preg_replace('/^(\w+:)?(\w+)$/','$2',$m[1]);

Copy link

Member

nicolas-grekasSep 23, 2016•
edited
Loading

Choose a reason for hiding this comment

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

instead of calling preg_replace in preg_replace_callback, I propose to change the previous regexp for:
'#^redis://(?:(?:[^:@]*+:)?([^@]*+)@)?#'
it should do the same, can you confirm?

Copy link

Member

nicolas-grekas commentedSep 23, 2016•
edited
Loading

OK, understood. There is a small potential for bc break if one uses a secret with a: inside, but it looks like DSN for Redis have this:password convention in many languages, not only PHP. We should adopt it.
Can you please rebase and change the base branch of this PR to 3.1?

ErikSaunier changed the base branch frommaster to3.1

September 23, 2016 13:35

[Cache] Fix password used to make the redis connection.

77eea43

Copy link

ContributorAuthor

ErikSaunier commentedSep 23, 2016•
edited by nicolas-grekas
Loading

Thank you@nicolas-grekas , it works with'#^redis://(?:(?:[^:@]*+:)?([^@]*+)@)?#'.
There is a very small potential of BC break for users with a: in their redis password but I also think we should adopt it.

Copy link

Member

nicolas-grekas commentedSep 23, 2016

👍

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels

Sep 23, 2016

Copy link

Contributor

jakzal commentedSep 23, 2016

👍

Copy link

Member

fabpot commentedSep 23, 2016

Thank you@ErikSaunier.

fabpot merged commit77eea43 intosymfony:3.1

Sep 23, 2016

fabpot added a commit that referenced this pull request

Sep 23, 2016

bug#20026[Cache] Fixed password used to make the redis connection. …

23eee70

…(ErikSaunier)This PR was merged into the 3.1 branch.Discussion----------[Cache] Fixed password used to make the redis connection.| Q             | A| ------------- | ---| Branch?       | 3.1| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#20025| License       | MIT| Doc PR        |http://symfony.com/blog/new-in-symfony-3-1-cache-componentI do not know if it's possible to provide a test as `REDIS_HOST` is provided by Travis in [RedisAdapterTest.php](https://github.com/symfony/symfony/blob/3.1/src/Symfony/Component/Cache/Tests/Adapter/RedisAdapterTest.php).Commits-------77eea43 [Cache] Fix password used to make the redis connection.

fabpot mentioned this pull request

Oct 3, 2016

Release v3.1.5#20144

Merged