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

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

Merged
merged 1 commit into from Sep 23, 2016
Merged

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

merged 1 commit into from Sep 23, 2016

Conversation

ErikSaunier
Copy link
Contributor

@ErikSaunier ErikSaunier commented Sep 22, 2016

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 as REDIS_HOST is provided by Travis in RedisAdapterTest.php.

@ErikSaunier 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 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
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 22, 2016

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

@ErikSaunier
Copy link
Contributor Author

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 in snc/SncRedisBundle RedisDsn.php for this case.

@@ -72,7 +72,7 @@ public static function createConnection($dsn, array $options = array())
}
$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

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?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 23, 2016

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 ErikSaunier changed the base branch from master to 3.1 September 23, 2016 13:35
@ErikSaunier
Copy link
Contributor Author

ErikSaunier commented Sep 23, 2016

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.

@nicolas-grekas
Copy link
Member

👍

@jakzal
Copy link
Contributor

jakzal commented Sep 23, 2016

👍

@fabpot
Copy link
Member

fabpot commented Sep 23, 2016

Thank you @ErikSaunier.

@fabpot fabpot merged commit 77eea43 into symfony:3.1 Sep 23, 2016
fabpot added a commit that referenced this pull request Sep 23, 2016
…(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-component

I 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 fabpot mentioned this pull request Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants