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

Environment Variables lack type - break built-in Symfony configuration #22151

Closed
smcjones opened this issue Mar 24, 2017 · 30 comments

Comments

Projects
None yet
@smcjones
Copy link

commented Mar 24, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 3.2

Related to #20434, if you try to pull data such as CACHE from an environment variable and pass it to Twig, Twig's Bundle reads the type literally, as "false" or "true" - as it is passed from getenv.

How to Replicate

Add this environment variable:

CACHE false

In config.yml:

twig:
    cache: %env(CACHE)%

This results in a cache being created under web/false/, which is certainly not the intended result.

Note that this has other unintended consequences throughout Symfony, where a boolean is expected but not validated.

Solutions?

A simple solution could be to use the env() library created by Laravel as it provides for type coercion out of the box.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

An environment variables is a string by design.

@smcjones

This comment has been minimized.

Copy link
Author

commented Mar 24, 2017

That is correct. However, Symfony configuration can be type aware. Therefore, there is a mismatch between what Symfony expects as a configuration parameter, and what Symfony provides as a configuration parameter through env().

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

Not really. Environment variables fits specific use cases.

The example you give is not a valid use case IMHO. Why would you want to change the cache value via an env var? Even if it was possible, that wouldn't work. As changing an evn var value does not triggers a container recompilation.

@smcjones

This comment has been minimized.

Copy link
Author

commented Mar 24, 2017

Symfony's stated methodology appears to disagree with you, as per this blog post which references environment variables as part of the twelve-factor app methodology.

http://symfony.com/blog/new-in-symfony-3-2-runtime-environment-variables

Why would you want to change the cache value via an env var?

The concept here is that I want to be able to switch debugging on and off for any given environment easily and quickly. However, my default for "prod" is going to be different from my default for "dev", where caching can be a nuisance. I do not want to rewrite my parameters.yml between environments - that is a big part of the point, and it's a big part of the point of why env() exists in 3.2.

As I write this, I realize that another solution is to add a parameters_dev.yml that holds cache: false. This is a good temporary measure.

If you don't like my example, though, please refer to literally any other place in Symfony where a boolean value is expected. Passing via %var% works fine. Passing via %env(var)%, on the other hand, does not. Perhaps debug is a more fitting example?

If you truly believe that there is no situation where an environment variable can or should be a boolean in Symfony's eyes, I ask you to reconsider carefully... This is an arbitrary limitation.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

Keep in mind that using env vars does not replace parameters. An env var is something that can change without the container being recompiled. Your example would not work even if booleans were supported.

@smcjones

This comment has been minimized.

Copy link
Author

commented Mar 24, 2017

I am not sure I am following you. I have been easily able to make this work in Symfony by creating a PHP configuration and validating the boolean manually myself. Laravel uses PHPDotEnv with their env() wrapper and it works fine. If you load %env(var)% in your configuration, either through parameters or otherwise, it is static throughout your application, and only changes when the container is recompiled.

I can certainly understand why there would be exceptions, or even valid reasons not to do this, but I do think it is a bug, especially for those of us who have used other frameworks. Symfony is incredible, but I do not believe that this should be the standard behavior.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2017

@smcjones

Keep in mind that using env vars does not replace parameters.

This is key; %foo% and %env(HOME)% are different things. For clarification;

parameters:
    foo: %env(HOME)%

services:
    foobar:
        class: stdClass
        properties:
            a: %foo%
            b: %env(HOME)%

Produces;

<parameter key="foo">%env(HOME)%</parameter>

<service id="foobar" class="stdClass">
  <property name="a">%env(HOME)%</property>
  <property name="b">%env(HOME)%</property>
</service>
$this->services['foobar'] = $instance = new \stdClass();

$instance->a = $this->getEnv('HOME');
$instance->b = $this->getEnv('HOME');

Parameters are resolved at compile time, env parameters at runtime. This is good!

https://symfony.com/blog/new-in-symfony-3-2-runtime-environment-variables also states that

Their main advantages are that they can be changed between deploys without changing any code

This is what's happening here.

However, i do understand the confusion; as it highly looks like normal parameters. But your solution of moving things to config_prod.yml is the way to go here.

Not sure SF should do anything here, other then improving docs. Maybe it's needs a different syntax to make it less confusing with parameters.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2017

@fabpot what do you think of the idea to support built-in type casts for env's?

%env:bool(CACHE)% => (bool) $this->getEnv('CACHE')

Looks like it fits a need; note this is not about string("false") => bool(false) conversions whatsoever. Meaning CACHE should be either "1" or "0".

@smcjones

This comment has been minimized.

Copy link
Author

commented Mar 25, 2017

@ro0NL I really do get the difference between compile-time and runtime. I am probably doing a poor job expressing this issue and my proposed fix. For me, the issue that leads to unnecessary confusion is this:

# envvar: CACHE=0

parameters:
    cache: %env(CACHE)%

foo: # foobundle asks for caching
    cache: %cache%

Configuration for FooBundle:

class Configuration implements ConfigurationInterface
{
    public function getConfigTreeBuilder()
    {
        $treeBuilder = new TreeBuilder();
        $rootNode = $treeBuilder->root('foo');

        $rootNode
            ->children()
                ->booleanNode('cache')
                    ->isRequired()
            // ...

    }
    // ....
}

This results in an error.

I am not suggesting that you force environment variables to be type coerced throughout Symfony - sorry if that was not clear. If you need that in your PHP files, you can simply use filter_var().

Proposed new behavior:

$this->getEnv('CACHE');   // (string) "false"
$this->getParameter('cache');  // (bool) false
filter_var($this->getEnv('CACHE'), FILTER_VALIDATE_BOOLEAN); // (bool) false

In my–possibly flawed–view, any time you call getenv() as a parameter, it would be great if it could be automatically filtered, as a refactored version and not a direct function call to $this->getEnv()

# envvar DEBUG=false
# parameter %debug% was set to %env(DEBUG)%
$this->getEnv('DEBUG'); // (string) "false"
$this->getParameter('debug'); // (bool) false
$this->getParameter('env(DEBUG)');  // (bool) false
putenv('DEBUG=true');
$this->getParameter('env(DEBUG)'); // (bool) true
$this->getEnv('DEBUG'); // (string) "true"
$this->getParameter('debug'); // still (bool) false

I hope this is clearer.

Also:

Note that if you run filter_var($var, FILTER_VALIDATE_BOOLEAN), the following strings will be evaluated as boolean:

"true"    // (bool)true
"1"         // (bool)true
"0"        // (bool)false
"false"  // (bool)false

This is built into PHP by design, and will prevent the breaking of config files that validate for a boolean value. Maybe I'm missing something (probably), but it does feel like validating for boolean should validate the intention and PHP standards, rather than the actual type, given these mismatches between environment variables and the way YML itself determines type. Requiring your cache to be named "web/false" seems like an extreme edge case. Additionally, if someone really wanted to do that, depending on how this was implemented it could wind up being doable like this:

cache: "%env(cache)%" # evaluates to "false" (string)

cache: %env(cache)% # evaluates to false (bool) 

I'm still relatively new to the Symfony universe, and I do truly find it a joy to use. If I'm suggesting something that flies in the face of the Symfony philosophy, I understand and humbly apologize. However, I want to make sure my point is at least understood as it was intended.

I do think simply updating the documentation to describe environment file handling would be a big help. I am sure there are other solutions as well, such as, in a Capistrano deploy for example, moving parameters.yml to the /shared folder and using it instead of phpdotenv.

Basically, it can be a bit of a struggle when you want more dynamic and fluid environment variables, and you don't want to make your application aware of and have to handle n Symfony environments.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2017

I get your point, and i think it's a serious DX issue (at least the confusion with parameters). So here to help :)

First thing; cache: %env(cache)%. This is deprecated YAML syntax; anything starting with % must be quoted in 4.0

User Deprecated: Not quoting the scalar "%env(CACHE)%" starting with the "%" indicator character is deprecated since Symfony 3.1 and will throw a ParseException in 4.0.

So what it evaluates to really depends on the type of the referenced value. And for env's this is always a string.

If i understand correctly you're proposing to automagically filter a parameter var, if it references a env var, right? Not sure about that, why exactly should we assume it's a boolean?

I think the conversion needs to be explicit, hence my proposal of doing it thru syntax;

%env(STRING)%
%env:bool(BOOL)%
%env:int(INT)%

This way BOOL=false could actually be supported using FILTER_VALIDATE_BOOLEAN like you mentioned.

This should also makes things compatible with PHP7 typehints for instance.

@smcjones

This comment has been minimized.

Copy link
Author

commented Mar 26, 2017

I think that makes a lot of sense! I am all for that solution.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 1, 2017

Reclassified as a feature request asking for typed env vars, which are not supported yet as explained by @fabpot
Somewhat related to #20276. If anyone wants to submit a PR to add support for this, please do.
The Config component would also need to be patched as part of the PR, as described in #22594.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented May 24, 2017

In case types are finally adopted for env vars, could we use Python's dictionaries syntax instead of the proposed syntax?

# Current Proposal
Strings: %env(...)%
Booleans: %env:bool(...)%
Integers: %env:int(...)%
Floats: %env:float(...)%

# Proposal based on Python's syntax
Strings: %env(...)s%  (%env(...)% works too)
Booleans: %env(...)b%
Integers: %env(...)i%
Floats: %env(...)f%
@tristanbes

This comment has been minimized.

Copy link

commented May 31, 2017

We should move toward a solution because right now we can't use dynamic config based on runtime for simple configuration (boolean)

Ex:

swiftmailer:
    disable_delivery: "%env(DISABLE_EMAIL_DELIVERY)%"
parameters:
    env(DISABLE_EMAIL_DELIVERY): true

Invalid type for path 'swiftmailer.mailers.default.disable_delivery'. Expected boolean, but got string

To me, it's unfortunate that 3.3 doesn't ship type hiting & conf validation :(

@tristanbes

This comment has been minimized.

Copy link

commented May 31, 2017

Also, could be related BUT in the following example, the bundle validates the jhg_nexmo.from_name with a alphanumeric constraint.

parameters.yml
    nexmo_from_name: '%env(NEXMO_FROM_NAME)%'
    env(NEXMO_FROM_NAME): "Project"


jhg_nexmo:
    from_name:  '%env(NEXMO_FROM_NAME)%'

It results when configuration being validated in :

[RuntimeException]                                                                                                                                                                                                                             
  An error occurred when executing the "'cache:clear --no-warmup'" command:                                                                                                                                                                      
  Fatal error: Uncaught InvalidArgumentException: Invalid from_name, only alphanumeric characters are allowed in /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php:204                             
  Stack trace:                                                                                                                                                                                                                                   
  #0 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php(256): Symfony\Component\Config\Definition\Builder\ExprBuilder->Symfony\Component\Config\Definition\Builder\{closure}('env_NEXMO_FROM_...')  
  #1 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/BaseNode.php(309): Symfony\Component\Config\Definition\Builder\ExprBuilder::Symfony\Component\Config\Definition\Builder\{closure}('env_NEXMO_FROM_...')             
  #2 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/ArrayNode.php(254): Symfony\Component\Config\Definition\BaseNode->finalize('env_NEXMO_FROM_...')                                                                    
  #3 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/BaseNode.php(303): Symfony\Component\Config\Definition\ArrayNode->finalizeValue(Array)                                                                              
  #4 /srv/app/ve in /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/BaseNode.php on line 313                                                                                                                             
  PHP Fatal error:  Uncaught InvalidArgumentException: Invalid from_name, only alphanumeric characters are allowed in /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php:204                        
  Stack trace:                                                                                                                                                                                                                                   
  #0 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php(256): Symfony\Component\Config\Definition\Builder\ExprBuilder->Symfony\Component\Config\Definition\Builder\{closure}('env_NEXMO_FROM_...')  
  #1 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/BaseNode.php(309): Symfony\Component\Config\Definition\Builder\ExprBuilder::Symfony\Component\Config\Definition\Builder\{closure}('env_NEXMO_FROM_...')             
  #2 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/ArrayNode.php(254): Symfony\Component\Config\Definition\BaseNode->finalize('env_NEXMO_FROM_...')                                                                    
  #3 /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/BaseNode.php(303): Symfony\Component\Config\Definition\ArrayNode->finalizeValue(Array)                                                                              
  #4 /srv/app/ve in /srv/app/vendor/symfony/symfony/src/Symfony/Component/Config/Definition/BaseNode.php on line 313                                   
@Chorss

This comment has been minimized.

Copy link

commented Jun 10, 2017

Hi, environment Variables don't work only in case

mailer_auth_mode
mailer_encryption

I'm getting an error

Invalid configuration for path "swiftmailer.mailers.default.auth_mode": The "%env(MAILER_AUTH_MODE)%" authentication mode is not supported

Symfony version 3.3.2
Replicate bug.
1.Run containers(Set env. variables)

docker-compose.yml

version: '3'

services:
symfony:
image: chorss/docker-extension-php:latest
volumes:
- "/path/to/your/folder/:/var/www/html"
links:
- db:mysql
ports:
- 80:80
environment:
- DATABASE_DRIVER=pdo_mysql
- DATABASE_HOST=mysql
- DATABASE_NAME=symfony
- DATABASE_USER=root
- DATABASE_PASSWORD=root
- DATABASE_CHARSET=UTF8
- DATABASE_PORT=3306
- MAILER_TRANSPORT=smtp
- MAILER_HOST=127.0.0.1
- MAILER_USER=null
- MAILER_PASSWORD=null
- MAILER_PORT=null
- SECRET_KEY=ThisTokenIsNotSoSecretChangeIt
- MAILER_AUTH_MODE=login
- MAILER_ENCRYPTION=ssl
db:
container_name: mysql
image: mysql:5.7
ports:
- 3306:3306
environment:
MYSQL_ROOT_PASSWORD: root

parameters.yml

parameters:
database_driver: '%env(DATABASE_DRIVER)%'
database_charset: '%env(DATABASE_CHARSET)%'
database_host: '%env(DATABASE_HOST)%'
database_port: '%env(DATABASE_PORT)%'
database_name: '%env(DATABASE_NAME)%'
database_user: '%env(DATABASE_USER)%'
database_password: '%env(DATABASE_PASSWORD)%'>
mailer_transport: '%env(MAILER_TRANSPORT)%'
mailer_auth_mode: '%env(MAILER_AUTH_MODE)%'
mailer_host: '%env(MAILER_HOST)%'
mailer_user: '%env(MAILER_USER)%'
mailer_password: '%env(MAILER_PASSWORD)%'
mailer_encryption: %env(MAILER_ENCRYPTION)%'
mailer_port: '%env(MAILER_PORT)%'
secret: '%env(SECRET_KEY)%'

  1. Run containers
  2. Enter

localhost/web/app_dev.php

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 11, 2017

@Chorss Which version of SwiftmailerBundle do you use?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 11, 2017

@Chorss this is unrelated to the current issue, but to SwiftmailerBundle. See symfony/swiftmailer-bundle#180

@tunh

This comment has been minimized.

Copy link

commented Aug 24, 2017

Is there anyway to work around environment variable data type? I want to pass some boolean value.

@ogizanagi

This comment has been minimized.

Copy link
Member

commented Aug 24, 2017

@tunh : See #23901

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2017

And #23888 :)

The workaround is to not use env params, but regular params for now. Or adjust your config constraints.

fabpot added a commit that referenced this issue Sep 7, 2017

feature #23901 [DI] Allow processing env vars (nicolas-grekas)
This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Allow processing env vars

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | see description
| License       | MIT
| Doc PR        | -

This PR is an updated version of #20276 ~~(it embeds #23899 for now.)~~

It superscedes/closes:
- [DI] Add support for secrets #23621 ping @dunglas
- Runtime container parameter not found event filter #23669 ping @marfillaster
- [DependencyInjection] [DX] Support for JSON string environment variables #23823 ping @Pierstoval
- add support for composite environment variables #17689 ping @greg0ire
- [DI] ENV parameters at runtime with PHP 7 strict types not working properly #20434 ping @sandrokeil
- Issue when using a SQLite database and the DATABASE_URL env var #23527 ping @javiereguiluz

#22151 is another story, so not fixed here.

The way it works is via `%env(foo:BAR)%` prefixes, where "foo" can be bound to any services you'd like.
By default, the following prefixes are supported:
- `bool`, `int`, `float`, `string`, `base64`
- `const` (for referencing PHP constants)
- `json` (supporting only json **arrays** for type predictability)
- `file` (eg for access to secrets stored in files.)
- `resolve` (for processing parameters inside env vars.)

New prefixes can be added by implementing the new `EnvProviderInterface`, and tagging with `container.env_provider` (see `Rot13EnvProvider` in tests.)

Prefixes can be combined to chain processing, eg.
`%env(json:base64:file:FOO)%` will be roughly equivalent to
`json_decode(base64_decode(file_get_content(getenv('FOO'))))`.

Commits
-------

1f92e45 [DI] Allow processing env vars
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 10, 2017

Would anyone like to try #23888? It might solve this issue!

@RedactedProfile

This comment has been minimized.

Copy link

commented Nov 10, 2017

Hey everyone,

I'm in a need to implement environment variables set through parameters configuration to be run through a Configuration.php

# config.yml
parameters:
    locale: en
    env(MY_VAR): true

bundleconfigurator:
    toplevelconfig:
      my_first_var:
        active: "%env(MY_VAR)%"
        // DependencyInjection/Configuration.php
        $treeBuilder = new TreeBuilder();
        $rootNode = $treeBuilder->root('bundleconfigurator');
        $rootNode
            ->children()
                ->arrayNode('toplevelconfig')
                    ->useAttributeAsKey('name')
                    ->prototype('array')
                        ->children()
                            ->booleanNode('active')
                                ->beforeNormalization()
                                    ->ifString()
                                        ->then(function($v) {
                                            if (substr($v, 0, 4) == "env_") {
                                                // environment variable comes in as string like "env_MY_VAR_2bb0ccb650f8cf0aa754dd8396513258"
                                                // I don't mind parsing this here to get the value, a bit hacky sure but I need it
                                                // I just don't know what to query to get the value of the registered env variable
                                                die(dump($v));
                                                return $v;
                                            }

                                            return boolval($v);
                                        })
                                ->end()
                                ->defaultFalse()

What can I do here exactly?

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2017

@RedactedProfile your work around is more or less what #23888 is aiming for out-of-the-box. With the exception you'd use %env(bool:MY_VAR)% in config. For now i think it's good practice to expect this value explicitly in config, or be more lax on the node type.

What can I do here exactly?

We are open for suggestions basically :-)

@jhkchan

This comment has been minimized.

Copy link

commented Dec 14, 2017

I am sharing my hack here. If you really want to get your expected type from env, here is what I do:

parameters:
    env(MY_BOOL): "b:1;"

any_service:
    trigger: !php/object env(MY_BOOL)

but I agree with @fabpot after I read his design, I updated my code and did not need this use-case anymore.

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 14, 2017

I'm closing this one as we do have support for casting values now.

@fabpot fabpot closed this Dec 14, 2017

@hakebi

This comment has been minimized.

Copy link

commented Dec 20, 2017

I still believe this is an issue, for example, in a fresh installation of symfony/skeleton if we edit our .env file and add APP_DEBUG=false, we will have a problem.

The public/index.php file will read APP_DEBUG and it will treat it as true, because as we know "false" as string evaluates true.

This means that there is actually no way of disabling debug with the env variable APP_DEBUG because it will be always treated as true by the index.php. The only way to disable debug is changing the APP_ENV.

Its true that normally we would not want to disable debug in dev env but I still think its an error to be able to set APP_DEBUG=false and that index.php treat it as true.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

@hakebi APP_DEBUG is one of those very early used env var that won't ever have any kind of preprocessing. Just use APP_DEBUG=0 in fact.

@ektarum

This comment has been minimized.

Copy link

commented Dec 26, 2017

@hakebi @nicolas-grekas it's not exactly the same problem.

the problem is in index.php

$debug = $_SERVER['APP_DEBUG'] ?? ('prod' !== $env);

if you declare APP_DEBUG in environment variable, $debug become a string and kernel wait a boolean...

code would be :

$debug = filter_var($_SERVER['APP_DEBUG'] ?? ('prod' !== $env), FILTER_VALIDATE_BOOLEAN);
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

Fixed in #23888

nicolas-grekas added a commit that referenced this issue Mar 27, 2018

feature #23888 [DI] Validate env vars in config (ro0NL)
This PR was squashed before being merged into the 4.1-dev branch (closes #23888).

Discussion
----------

[DI] Validate env vars in config

| Q             | A
| ------------- | ---
| Branch?       | 4.1/master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22151, #25868
| License       | MIT
| Doc PR        | symfony/symfony-docs#8382

This PR registers the env placeholders in `Config\BaseNode` with its default value or an empty string. It doesnt request real env vars during compilation,

What it does is if a config value exactly matches a env placeholder, we validate/normalize the default value/empty string but we keep returning the env placeholder as usual. If a placeholder occurs in the middle of a string it also proceeds as usual.

The latter to me is OK as you need to expect any string value during runtime anyway,  including the empty string.

Commits
-------

2c74fbc [DI] Validate env vars in config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.