Skip to content

[DX] Static vs. runtime env vars #40794

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

Open
webmozart opened this issue Apr 13, 2021 · 47 comments
Open

[DX] Static vs. runtime env vars #40794

webmozart opened this issue Apr 13, 2021 · 47 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Help wanted Issues and PRs which are looking for volunteers to complete them. Keep open

Comments

@webmozart
Copy link
Contributor

webmozart commented Apr 13, 2021

Description

Since switching Symfony to env vars, there is a systematic issue with bundle configurations. Env vars are replaced by string placeholders when compiling the container and those placeholders will be passed to bundle configurations. Any configuration value that expects anything else but a string will fail with an error like:

Invalid type for path "web_profiler.toolbar". Expected boolean, but got string.

A quick research shows up several bug reports of this kind:

Benefits of env vars

Env vars as used in Symfony provide two benefits:

  1. Configure the application with static values provided in .env, .env.local etc. Those are known during container compilation.
  2. Configure the application with dynamic values provided by the runtime (Apache, Nginx). Those are not known during container compilation.

It is officially recommended to dump the environment variables to a PHP file in production. Hence my assumption would be that the majority of Symfony users is using mostly static variables.

Since static variables are known during container compilation, those could be passed to bundle configurations without ever changing a bundle to "support env vars".

Proposal: Static vs. runtime env vars

I propose to distinguish between static and runtime env vars. Static env vars (Group 1) are resolved at compile time and statically compiled into the container. Runtime env vars (Group 2) are replaced by placeholders and resolved at runtime.

For BC, all env vars are regarded as resolved at runtime. With a switch (e.g. framework.static_env_vars), all env vars can be switched to static.

When individual variables are required to be resolved at runtime, those could be marked with a runtime: processor. Such variables are resolved at runtime even if static_env_vars is set to true.

In a later major release, I propose to flip static_env_vars to true by default to improve DX.

Example

framework:
    static_env_vars: true

some_bundle_config:
    setting1: "%env(SETTING_1)%" # resolved at compile time - compiled into the container
    setting2: "%env(runtime:SETTING_2)%" # resolved at runtime - placeholders are passed to bundle configuration

Benefits

  • Only one configuration format (.env) rather than two (.env and parameters.yaml) for deployment specific configuration
  • Flat learning curve: Start with static env vars, introduce runtime env vars gradually when needed
  • Supports secrets for all settings (secrets are not supported for DI parameters)
  • Supports locally overriding settings in .env.local (not easily achievable with parameters.yaml)
@carsonbot carsonbot added the DX DX = Developer eXperience (anything that improves the experience of using Symfony) label Apr 13, 2021
@nicolas-grekas
Copy link
Member

Good to see you here @webmozart :)
We already have this distinction in core: parameters are for static values, and env vars for runtime-configurable values. I know ppl are abusing env vars, but adding a third concept won't help I fear.

@stof
Copy link
Member

stof commented Apr 13, 2021

@nicolas-grekas I think this confusion comes from the fact that the vcs-ignored parameters.yml file has been removed from the flex skeleton (compared to the standard-edition skeleton). this means that by default, you have 2 choices: env variables or vcs-managed parameters. But there needs to be a room for per-setup static values (where various deployments might need a different value for the parameters).

@webmozart my solution to this problem is actually to reintroduce the parameters.yml file in my project for static values, managed with https://packagist.org/packages/incenteev/composer-parameter-handler (disclaimer: I'm the maintainer of that package), as done in the old symfony standard-edition skeleton.

@Ocramius
Copy link
Contributor

Ocramius commented Apr 13, 2021

It is officially recommended to dump the environment variables to a PHP file in production. Hence my assumption would be that the majority of Symfony users is using mostly static variables.

Overall, this approach is not endorsable, and goes against https://12factor.net/codebase (one source, multiple deployments), which is sensible when you have one application to be re-configured differently depending on, as it may be surprising, the environment.

I only use ENV vars, and also removed bindings for symfony/dotenv from bin/console and public/index.php in each system I maintain, precisely to avoid this confusion (as @nicolas-grekas just stated, it is an additional concept).

What remains is the annoyance of running tasks like docker build or composer install within scopes where the environment is not fully bootstrapped, highlighting that sometimes the environment variables are being read even when they should only be referenced (too eager container build process - this should be treated as a bug).

The workaround I found to be working for this is to define the environment variables with defaults that don't affect the system too much:

# put this anywhere you want in your configs
parameters:
    # empty on purpose - hides absence of env variable
    env(DATABASE_URL): ''
    # or put a fake value in it - I'm a github comment, not a cop
    #env(DATABASE_URL): 'mysql://localhost'

doctrine:
    dbal:
        # use your env vars as usual
        url: '%env(my:preprocessors:here:DATABASE_URL)%'

Alternatively, declare ENV with neutral values within your Dockerfile, in the build process.

This requires a lot of dancing around changes in upstream bundles, but seems to work quite reliably for the build process. Ideally, environment variables should not be touched during compilation steps, but mistakes in upstream are not always avoidable.

@stof
Copy link
Member

stof commented Apr 13, 2021

@Ocramius but then, this highlights the need for a way to specify build-specific parameters in a different way than env variables, for settings that must affect the way the container is built. Bundle configurations are not always passed as is to service arguments to be used at runtime. They can also affect the way services are wired (and in such case, env placeholders don't work)

@Ocramius
Copy link
Contributor

Ocramius commented Apr 13, 2021

They can also affect the way services are wired

Generally best to use a runtime factory, if such a runtime decision is needed 🤔

What sometimes happens is:

  • build process attempts to access a service (logger, for example)
  • build process attempts to read database schema

These are supposed to not happen during build time at all. Perhaps what's needed is preventing usage of %env()% when a parameter is to be used in an extension, by having a bundle/container extension declare when a value is accepted as dynamic, and otherwise they should all be considered static by default? (BC break, obviously - lots of reasoning to be done there).

In practice, if I do following, and the bundle did NOT explicitly declare use_profiler as dynamic:

  • use_profiler: %env(USE_THE_PROFILER)%

Then what I could get as useful feedback is a DynamicValueUsedInStaticExpression exception (thrown by the DIC build process).

This could potentially allow for more aggressive inlining too.

@alexander-schranz
Copy link
Contributor

@webmozart

Invalid type for path "web_profiler.toolbar". Expected boolean, but got string.

Got the same problem here when using PrependExtension: #40198. I could only solve a part of it here, but not the usage with env variables yet.

I understand the point with parameters vs env vars, but could not also bool values be this type of env vars, because for me currently most of the time it looks like only string value can be env vars and the dependency injection can not work if it is bool, int, float or other typed values.

@stof
Copy link
Member

stof commented Apr 13, 2021

@Ocramius requiring bundles to declare explicitly in their configuration which settings support env placeholders and which don't support it is a separate topic (it would indeed provide better feedfack when using an env placeholder in an unsupported place, but it is hard to introduce in a BC way as we would have to support an intermediate state where bundles have not updated their config definition yet). But that's not solving this issue if we don't provide an official way to configure static values

@stof
Copy link
Member

stof commented Apr 13, 2021

@alexander-schranz if you need a boolean value for which the value is only needed at runtime, you can do it with env placeholders, by using %env(bool:MY_ENV_VAR)%

@alexander-schranz
Copy link
Contributor

@stof but this doesn't work when the value is given from a bundle as value to a PrependExtension which only expects a boolValue when the extension is parsed with:

        $resolvingBag = $container->getParameterBag();
        $configs = $resolvingBag->resolveValue($configs);

it fails with Expected "bool", but got "string". See also here.

@webmozart
Copy link
Contributor Author

webmozart commented Apr 13, 2021

Overall, this approach is not endorsable, and goes against https://12factor.net/codebase (one source, multiple deployments),

I don't think that's true. The .env.local file should not be included in VCS and dumping should happen in the deployment environment, not before committing to VCS.

For other readers: @nicolas-grekas pointed to his presentation on Twitter that I find quite informative: https://speakerdeck.com/nicolasgrekas/configuring-symfony-from-localhost-to-high-availability

The gist is:

  • Use (DI) parameters for business requirements
  • Use env vars for infra-related configuration
  • Use .env files first, increase complexity later
  • Use bin/console secrets:*

There are various problems with this approach:

  • Bundle authors need to explicitly support env vars in configuration that you consider infrastructure-related, even if that configuration does not depend on the runtime
  • Some business requirements settings may need to be protected, but secrets are only supported for env vars
  • Some business requirements vary from deploy host to host (same app installed for different clients)

Especially for the last reason, the parameters.yml needs to be reintroduced (as recommended by @stof) and integrated in the deploy process, i.e. during deploy, we need to build a parameters.yml with business settings and a .env.local with infrastructure settings. This isn't very simple, let alone for new users.

Using static env vars by default would flatten the learning curve:

  1. Use .env and .env.local for all configuration that varies from deploy host to deploy host (both infrastructure and business)
  2. Run composer dump-env for performance optimization
  3. Once you scale out and need dynamic information from the runtime, change %env(XXX)% to %env(runtime:XXX)% only where you need it

Much simpler IMO.

@webmozart webmozart changed the title [DX] Lazy vs. non-lazy env vars [DX] Static vs. runtime env vars Apr 13, 2021
@alexander-schranz
Copy link
Contributor

@webmozart Should not the bundle define which env variables would be able to be changed at runtime and which are directly resolved when the container is build e.g.:

                        ->booleanNode('enabled') // config option which does not support runtime change of env
                        ->end()
                        
                        ->booleanNode('someValue')
                            ->lazyEnv() // or ->runtimeVariable()
                        ->end()

Because I think if the bundle does something like the lazy: can not be used:

if ($config['enabled'])) { // when env is used the resolved env variable is given here so this will always be bool with true or false and no call for resolving itself is needed
     $loader->load('some-services.xml');
}

// for the config lazyEnv the env placeholder is given here so its changeable on runtime
$container->setParameter('some_env', $config['someValue']);

At this case the bundle will always get the resolved values and only defines the config values which are changeable at runtime be given to the bundle as env placeholders.

@webmozart
Copy link
Contributor Author

webmozart commented Apr 13, 2021

@alexander-schranz Yes, although I would probably take the opposite approach and only mark those configuration options that explicitly don't support runtime variables. The status quo is that bundles will fail anyway if you pass runtime variables in those options. Adding validate()->noRuntime()->end() or similar will simply provide a better error message.

Requiring all bundles that already support runtime variables today to add runtimeVariable() flags is a BC break.

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Apr 13, 2021

@webmozart the problem I see there is that if we automatical say all config options are possible as env vars something like this will fail:

if ($config['enabled'])) { 
     $loader->load('some-services.xml');
}

The $config['enabled'] will return by symfony in the following format like __env1413512124 (not remember the format) also when set to false when e.g.: the config and env var is:

MY_ENV_VAR=false
my_config:
     enabled: "%env(bool:MY_ENV_VAR)%"

So I think it would better to define for which $config variable I expect that symfony does not resolve the env variables instead. I think this would make it a lot better for bundle developers as they will not longer get an unexpected value for some config parameter.

Requiring all bundles that already support runtime variables today to add runtimeVariable() flags is a BC break.

Sure this is a BC break and could only be done in the next release or can be enabled over a flag in the config rootTree.

@dkarlovi
Copy link
Contributor

The main design issue is Symfony doesn't have a distinct build vs run phase, even though it obviously has one implicitly.

Having a build phase would mean requiring a reference to a env var to be fully resolved would lead to an exception because env variables are not available during the build phase.

We discussed this in some detail in Symfony Slack already, it was dismissed then too.

@gggeek
Copy link

gggeek commented Apr 18, 2021

I'm can't say for sure that I grasp fully the scope of this, nor I am fully up to date with what the best practices are or the most commonly used patterns, but here's my feedback:

  • the situation atm with Symfony config is imho quite complicated, possibly too much. Flexible, for sure, but I find myself spending hours hunting down the exact file / env var / build script which is the origin of a specific value (I mostly develop on ezplatform, which abuses symfony config a lot, but I am sure it is not the only case)
  • having env vars which can be declared/overridden in .env/.env.local files adds a certain amount mental burden, as env vars can be injected via other means, and then the problem of priority pops up (real env vars vs .env permutations)
  • adding a lot of mental burden is: trying to figure out which vars/settings get resolved at container compile time, and which ones at runtime. If I make all of the settings depend on parameters, and I set the value of those parameters using env vars, will the app become dead slow?

The proposal from @webmozart seems to me to add one more layer of complexity adding a concept which is quite unusual "static env vars" as it clashes with the way the rest of the OS works.
Maybe it could be rewritten in a way to make more sense to the laymen by distinguishing compile-time parameters from runtime parameters ? (*)

More specifically:

Bundle authors need to explicitly support env vars in configuration that you consider infrastructure-related, even if that configuration does not depend on the runtime

I thought that bundle authors would only have to ever support parameters to drive the config of their bundles, and the app developers would be left with the task of having config that sets the values of those from env var values.

(*) = a similar scenario: in my docker-compose builds I have come to use a single .env file to declare env vars which are used to configure everything, from mount volumes paths, to versions of php in use, to ports to expose, etc... All of the vars are used in docker-compose.yaml, some being passed on to the containers as env vars, some as variables to the dockerfiles, and hence only usable at image build time. The simplicity of having a single config file is unrivaled. However, when changing one var value in the .env file, sometimes a container restart is enough, sometimes a rebuild is needed. I found no better way to surface this requirement than making the .env file heavily documented...

@ro0NL
Copy link
Contributor

ro0NL commented Apr 18, 2021

adding a lot of mental burden is: trying to figure out which vars/settings get resolved at container compile time, and which ones at runtime

IIUC that be

debug:container --env-vars from the container POV
debug:config --resolve-env (#40582) from the config POV

@gggeek
Copy link

gggeek commented Apr 18, 2021

@ro0NL thx, but those are not available in all Smfony versions I am using (currently migrating one app from 2.8 to 3.4...)

@stof
Copy link
Member

stof commented Apr 20, 2021

@gggeek 2.8 does not support env variables at all...

@gggeek
Copy link

gggeek commented Apr 20, 2021

@stof yep. Otoh 3 does, and that's where the story about parameters started to get very complicated (imho of course)

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@webmozart
Copy link
Contributor Author

@carsonbot From my POV this is still open. In one of our projects, we ditched environment variables completely and reintroduced parameters.yaml to make this manageable for our developers. That's not really a solution though.

I reread the thread and still think my approach would facilitate the problem.

  • At build time, all environment variables from the current environment are compiled into the container by default. I.e. if you deploy and warm-up the cache on the remote machine, the configuration of that machine is used.
  • If some variables should stay dynamic (AWS deployment, Apache2 overrides for some reason etc.), those can be explicitly marked as runtime:.

A simple approach that (mostly) fixes the issue without reinventing the way that bundles process configuration.

@carsonbot carsonbot removed the Stalled label Oct 21, 2021
@ro0NL
Copy link
Contributor

ro0NL commented Dec 18, 2021

I would consider the reverse route instead, thus a compile: prefix. Another question i'd have is, should the resolved value be compiled into the dumped container actually. I dont think it's possible,

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Mar 31, 2022

@dkarlovi why it is not recommended it is still possible by doing $container->resolveEnvPlaceholders($yourconfigParam, true);:

We @sulu are using it to check if ghostscript executable is correctly installed.

https://github.com/sulu/sulu/blob/08863981d9e607dd1432db68eec2dac710732f74/src/Sulu/Bundle/MediaBundle/DependencyInjection/SuluMediaExtension.php#L267

Keep in mind changing the env then also requires a cache clear. And that is what envs normally don't require if they are just used inside a service.

@nicolas-grekas dedicated syntax sounds like a great solution as that would also work for third party bundles which not always support using for all configs env vars, would go with static:.

@dkarlovi
Copy link
Contributor

@alexander-schranz I realize this and think it's OK, you have a special case and are using special API prepared for that purpose. What I'm talking about is bundles doing it by accident and not realizing because their test setup includes the env var which gets read, but it actually shouldn't be, example:
snc/SncRedisBundle#434
#37069

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 1, 2022

Because of env var processors, and most importantly because of env var loaders, resolving static env var is going to have to create service instances at compile time. This is something that might fail. Eg when resolving a static env vars would mean connecting to a Consul DB that is configured by a regular (dynamic) env var, there should be an error. There might be some more difficulties. To be discovered by trying.

I'm puting the "Help wanted" label on this issue. PR welcome if any would like to give it a try.

@valtzu
Copy link
Contributor

valtzu commented Feb 7, 2023

I found this interesting and gave it a try.

My approach:

  1. Build-time, iterate over env vars used in DI
  2. For the ones having static: prefix, put those in the parameter bag, just like normal env vars (as env(VAR_NAME))
  3. At runtime, for static: prefixed env vars, only use values from the parameter bag

This approach should be pretty compatible with extensions & custom env var processors.

The downside is that we are not getting a performance boost as env var processors are still executed during runtime.

valtzu@17e56f5

I tested that with a service where I injected %env(static:STATIC_PARAM)%, then tried changing it runtime – no effect (as expected). bin/console debug:container --env-vars still fails though

Looks like it's more complex than I initially thought 😄

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 12, 2023

When an env var is dynamic, the host is the part that knows about it. With this in mind, using prefixes as discussed above (%env(static:FOO)%, %env(runtime:FOO)%) feels like putting a tool in the wrong hands. Instead of allowing the configuration to specify which env vars are static/dynamic via those prefixes, we could take an approach that allows the host to say which env vars are dynamic.

Here is a proposal:

We make this the default configuration:

framework:
    dynamic_env_vars: '%env(default::SYMFONY_DYNAMIC_ENV_VARS)%'

And we define this:

  • when framework.dynamic_env_vars is null, all env vars are considered dynamic (current behavior);
  • otherwise, framework.dynamic_env_vars is an anchored regular expression;
  • framework.dynamic_env_vars defaults to the SYMFONY_DYNAMIC_ENV_VARS, which is a compile-time only env var.

When framework.dynamic_env_vars is defined, we inline the env vars that do not match the regexp if they are defined at compilation time. If a referenced env var is either not defined at compilation time, or if it matches the regexp, then we don't inline it.

Then we patch the default recipe to set SYMFONY_DYNAMIC_ENV_VARS='/-/' for new projects, so that by default env vars are all considered static. Projects that are deployed on high-availability hosts would then need to define either SYMFONY_DYNAMIC_ENV_VARS or framework.dynamic_env_vars to their need.

@webmozart
Copy link
Contributor Author

webmozart commented Jul 12, 2023

@nicolas-grekas I like it! The only downside I see is that it will be very tricky to find out why an env var set in the system environment is ignored by Symfony (since, even if only required in some cases, it is what a developer would expect).

Maybe it could be helpful to add corresponding checks when debug mode is enabled or something similar. For example, Symfony could check if an env variable referenced by the app configuration is provided by the system but not included in SYMFONY_DYNAMIC_ENV_VARS and log a warning otherwise:

WARNING: The environment variable "APP_HOST" is set in the system environment but not whitelisted in 
"SYMFONY_DYNAMIC_ENV_VARS". Its value "217.23.210.6" is ignored.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 12, 2023

are we really sure this is the way, compared to just using static %param% where needed 😅

@valtzu
Copy link
Contributor

valtzu commented Jul 12, 2023

At least for me the biggest headache currently is just that it's not easy to figure out what can be changed runtime and what can't.

all env vars are considered dynamic (current behavior)

Currently popular extensions like monolog are resolving env vars build-time, making this statement untrue from developer's perspective. Whether that's intended usage in an extension or not, I don't expect things like that to ever go away – so in my opinion most helpful would be to just have some way to easily see if %env(ENV_VAR)% can be changed run-time or not, something like this. It's rather simple, just tracks all the resolved env vars during the container build.


As mentioned in the PR:

In my opinion better solution would be to deny the usage of resolveEnvPlaceholders before container is built but that would of course break backwards compatibility.

@ostrolucky
Copy link
Contributor

I agree with the assessment that it's not clear which values can be dynamic and which can't. I've run into that issue in #49956 or snc/SncRedisBundle#483 Somehow warning users on this would be good first improvement. But I'm not sure how would this be technically detected.

@nicolas-grekas
Copy link
Member

There's another logical mistake with the "prefix" proposals BTW: they put the static/dynamic info on the reference (%env()% is a reference), whereas what we want is to label the env vars themselves as dynamic/static. We don't want an env var to be referenced in one place as static and in another as dynamic.

WARNING: The environment variable "APP_HOST" is set in the system environment but not whitelisted in
"SYMFONY_DYNAMIC_ENV_VARS". Its value "217.23.210.6" is ignored.

You might have misunderstood my proposal: real env vars not listed in SYMFONY_DYNAMIC_ENV_VARS would still be read at compile-time, and inlined. So this situation won't exist.

are we really sure this is the way, compared to just using static %param% where needed sweat_smile

You're ignoring the full discussion, unless I didn't understand it 🤷
What is asked for is a way to consider some / all env vars as static by default, this meaning that the value to use is provided by then host at compile-time through an env var, whose value is static and can thus be inlined.
Parameters don't help with this, unless you add something like @stof's incenteev/composer-parameter-handler in the process.

So yes, that's my best proposal :)

At least for me the biggest headache currently is just that it's not easy to figure out what can be changed runtime and what can't.

That will be easy to implement as a command with the configuration I'm proposing. Yes, it's already possible to inline env vars at compile time, but this is at the discretion of the bundle (like Monolog) and is certainly not the default nor the most common. This proposal would reverse the logic, making env vars work everywhere by default and opt-in to dynamic mode as needed.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 13, 2023

What is asked for is a way to consider some / all env vars as static by default,

but why actually? what's wrong with explicit %env(DYNAMIC)% v. %static%? or am i missing it?

AFAIK incenteev/composer-parameter-handler is just a tool to manage local parameters from dist. it's optional in the process, but i like to have this for .env files sure.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 13, 2023

@ro0NL you're missing the discovery part of env vars. %static% needs a value, and this value can be provided by the host as an env var. Generating a param.yaml file might fit your deployment strategy, but not everybody is doing that this way. Env vars are perfectly fine for that purpose also.

@webmozart
Copy link
Contributor Author

@nicolas-grekas I understood your proposal, but apparently I was unclear. When all env vars are inlined by default, and future developers try to override them in the system (after compilation, e.g. in HA environments), they will be confused that it doesn't work (DX--). It would be great if we would catch them there and point them to the fact that they need to add the var to SYMFONY_DYNAMIC_ENV_VARS.

Of course, making that very clear in the docs will help to some degree.

@Seldaek
Copy link
Member

Seldaek commented Jul 13, 2023

When all env vars are inlined by default, and future developers try to override them in the system (after compilation, e.g. in HA environments), they will be confused that it doesn't work (DX--).

I'm wondering if one way to mitigate this might be a new API to get parameters from the container, like getStaticParameter or getInlinedParameter or something.. Then that function could throw if you are fetching a dynamic env var. Then at least bundles like monolog that may have use cases where keeping dynamic env vars all the way to the runtime conf is difficult would be able to use this new method and make it clearly blow up at runtime. It's not amazing DX but it's still better than an implicit confusing behavior people have to debug themselves.

@nicolas-grekas
Copy link
Member

@valtzu you stated that "popular extensions like monolog are resolving env vars build-time", but I can't verify this statement by looking at the code of either monolog nor monolog-bundle. Could you please give some pointers so that I can get what you mean?

@webmozart got it thanks for the precision. The question I have then is when should this check happen? Having it on every single request looks like significant overhead to me. We could have it when e.g. cache:warmup is run but mmmh. Or good doc yes! I'm expecting ppl with HA needs to be able to figure this out (could be done once for all by hosting providers so ppl might not need to have to discover this by themselves also.)

@Seldaek I'm not convinced this would help yet: what happens in practice is that ppl might try to use an env var in some config option that doesn't support it. And they get an error. I don't think Symfony needs help from bundle authors here as the current behavior provides all the needed context. I might be wrong of course, future will tell IMHO.

@valtzu
Copy link
Contributor

valtzu commented Jul 13, 2023

@nicolas-grekas sure.

monolog.yaml

monolog:
  handlers:
    stderr:
      type: stream
      path: "php://stderr"
      level: '%env(LOG_LEVEL)%'
      process_psr_3_messages: false      

Then run f.e. LOG_LEVEL=250 bin/console so the container will be built.

App_KernelDevDebugContainer.php

    protected function getMonolog_Handler_StderrService()
    {
        return $this->privates['monolog.handler.stderr'] = new \Monolog\Handler\StreamHandler('php://stderr', 250, true, NULL, false);
    }

Instead of that 250, it should compile into $this->getEnv('int:LOG_LEVEL') to make runtime config possible, but here the issue may be that it needs to also do some conversion, i.e. notice -> 250 (which you probably could do with env var processor).

Stack trace:

  1. ContainerBuilder::resolveEnvPlaceholders
  2. MonologExtension::levelToMonologConst
  3. MonologExtension::buildHandler
  4. MonologExtension::load
  5. MergeExtensionConfigurationPass

Is this an unintended side effect of MergeExtensionConfigurationPass after all 🤔

@webmozart
Copy link
Contributor Author

webmozart commented Jul 13, 2023

The question I have then is when should this check happen?

@nicolas-grekas Maybe the solution is less to check it than to communicate it in places that developers are likely to stumble across when debugging. For example, in debug:dotenv:

Variables
---------

---------- ------- ---------- ------ ---------------
 Variable   Value   .env.dev   .env   Environment
---------- ------- ---------- ------ ---------------
 FOO        BAR     n/a        BAR    not supported
 ALICE      BOB     BOB        bob    alice
---------- ------- ---------- ------ ---------------

Tip: Change SYMFONY_DYNAMIC_ENV_VARS to support overriding variables in the system environment.

An additional place could be dotenv:dump:

The following variables can be overridden in the system environment:

* FOO
* ALICE

Adapt SYMFONY_DYNAMIC_ENV_VARS to add or remove variables from this list.

Finally, a similar comment could be added to .env.local.php in case somebody inspects the file for debugging without running the console commands.

@nicolas-grekas
Copy link
Member

@valtzu thanks for the pointers, I missed the call to resolveEnvPlaceholders. I feel like implementing my latest proposal would allow removing this kind of logic. 🎉

@webmozart good ideas thanks for helping shaping this :)

@ihmels
Copy link
Contributor

ihmels commented Jul 13, 2023

In Symfony environment variables not only serve as runtime environment variables, but are also used as build-time environment variables. In my opinion this should be fixed in the long term rather than making environment variables build-time environment variables by default.

I'm not a fan of this SYMFONY_DYNAMIC_ENV_VARS variable or this mechanism, because then you have to opt-in to what is actually intended behavior.

I realize that there is a requirement to control the build of the container via environment variables. However, one should then enable inlining of environment variables (see #40794 (comment)) instead of disabling inlining in all other cases. It is the application (and not the host) that has the knowledge whether an environment variable needs to be inlined or not.

@dkarlovi
Copy link
Contributor

dkarlovi commented Aug 2, 2023

Whatever the chosen solution is, it would hopefully help with this scenario:
#50322 (comment)

The idea should be build time processing of runtime information could help detect corner cases like this in some way upstream, telling (in this case DoctrineBundle) the callee their runtime is unexpectedly happening at build time.

@victortodoran
Copy link

Hello all

Is there any update on this? Is anyone working on a solution/solution design or is there an existing blocker?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Help wanted Issues and PRs which are looking for volunteers to complete them. Keep open
Projects
None yet
Development

No branches or pull requests