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

Add support for REDIS_URL environment variables. #20891

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@robinvdvleuten
Contributor

robinvdvleuten commented Dec 13, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20850
License MIT
Doc PR -

In most PAAS solutions - Heroku for example - the Redis connection string is exposed as an environment variable like REDIS_URL. This PR adds support for those by allowing the following config;

framework:
    cache:
        default_redis_provider: "%env(REDIS_URL)%"

In the referenced issue there was some discussion about maybe a new config options like default_redis_url, but this makes it hard to check in the extension for the case that an user configured a custom default_redis_provider and also has configured the default url.

@stof

This comment has been minimized.

Member

stof commented Dec 13, 2016

Can you add a test covering this ?

@robinvdvleuten

This comment has been minimized.

Contributor

robinvdvleuten commented Dec 13, 2016

@stof sure! Just a sec then

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 13, 2016

That won't work: injecting a string (the REDIS_URL) in RedisAdapter is not allowed.
What you need to do is add a new config option, let's say default_redis_url, and use that to wire the REDIS_URL as a DSN in the same way that I suggested in #20850 (comment) (but programmatically in the DI extension.)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 13, 2016

I should review twice before posting :) So this forces any %env(...)% to be understood as a DSN. OK then :)
Does this really work? I mean, env vars are usually seen as unique placeholders from DI ext., not a plain %env()% thing.
Adding a test case will show for sure.

@robinvdvleuten

This comment has been minimized.

Contributor

robinvdvleuten commented Dec 13, 2016

@nicolas-grekas what I am trying to understand is why the service definition you gave in the ticket;

cache.default_redis_provider:
    class: Redis
    public: false
    factory: ["Symfony\\Component\\Cache\\Adapter\\RedisAdapter", "createConnection"]
    arguments:
      - "%env(REDIS_URL)%"

is different from this in PHP;

$definition = new Definition(\Redis::class);
$definition->setPublic(false);
$definition->setFactory(array(RedisAdapter::class, 'createConnection'));
$definition->setArguments(array('%env(REDIS_URL)%'));
$container->setDefinition($name, $definition);

This should do the same in the compilation process right?

@robinvdvleuten

This comment has been minimized.

Contributor

robinvdvleuten commented Dec 13, 2016

@nicolas-grekas ha you typed a response faster than I did. This approach worked in my application so I created this PR to get your opinion before putting any effort in the test cases. The default_redis_provider isn't testing at all btw.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 13, 2016

When I apply your patch, I get the same:

[Symfony\Component\DependencyInjection\Exception\EnvParameterException]             
Incompatible use of dynamic environment variables "REDIS_URL" found in parameters.  

The correct patch might be more like:

--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CachePoolPass.php
+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CachePoolPass.php
@@ -106,7 +106,9 @@ class CachePoolPass implements CompilerPassInterface
      */
     public static function getServiceProvider(ContainerBuilder $container, $name)
     {
-        if (0 === strpos($name, 'redis://')) {
+        $container->resolveEnvPlaceholders($name, null, $usedEnvs);
+
+        if (0 === strpos($name, 'redis://') || $usedEnvs) {
             $dsn = $name;
 
             if (!$container->hasDefinition($name = md5($dsn))) {

Btw, I'd be OK on my side to have this as bugfix on 3.2.

@robinvdvleuten

This comment has been minimized.

Contributor

robinvdvleuten commented Dec 13, 2016

@nicolas-grekas you are right, I was looking at the wrong thing. I've added the resolve call and updated the testcases to test both the default string and the env var.

framework:
cache:
default_redis_provider: "%env(REDIS_URL)%"

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 13, 2016

Member

missing EOL

This comment has been minimized.

@robinvdvleuten

robinvdvleuten Dec 13, 2016

Contributor

Added the EOL

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 13, 2016

👍 (on 3.2 also)

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 13, 2016

Thank you @robinvdvleuten.

fabpot added a commit that referenced this pull request Dec 13, 2016

bug #20891 Add support for REDIS_URL environment variables. (robinvdv…
…leuten)

This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes #20891).

Discussion
----------

Add support for REDIS_URL environment variables.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20850
| License       | MIT
| Doc PR        | -

In most PAAS solutions - Heroku for example - the Redis connection string is exposed as an environment variable like `REDIS_URL`. This PR adds support for those by allowing the following config;

```yml
framework:
    cache:
        default_redis_provider: "%env(REDIS_URL)%"
```

In the referenced issue there was some discussion about maybe a new config options like `default_redis_url`, but this makes it hard to check in the extension for the case that an user configured a custom _default_redis_provider_ and also has configured the default url.

Commits
-------

4e6086f Add support for REDIS_URL environment variables.

@fabpot fabpot closed this Dec 13, 2016

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone Dec 13, 2016

@robinvdvleuten robinvdvleuten deleted the robinvdvleuten:redis-as-env-var branch Dec 13, 2016

@fabpot fabpot referenced this pull request Dec 13, 2016

Merged

Release v3.2.1 #20897

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 13, 2016

This is the fastest ever submitted+reviewed+fixed+merged+released PR :)

@robinvdvleuten

This comment has been minimized.

Contributor

robinvdvleuten commented Dec 13, 2016

@nicolas-grekas yes indeed! Thanks for all the time and effort you guys put into this process :)

@dunglas

This comment has been minimized.

Member

dunglas commented Dec 13, 2016

However, it seems that it breaks tests: https://travis-ci.org/symfony/symfony/jobs/183649383

@dunglas

This comment has been minimized.

Member

dunglas commented Dec 13, 2016

Sorry, this isn't related: #20906

@chalasr

This comment has been minimized.

Member

chalasr commented Dec 15, 2016

I do think it's related since the test only fails in its redis version with a custom config, it succeeds otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment