-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Comments
Good to see you here @webmozart :) |
@nicolas-grekas I think this confusion comes from the fact that the vcs-ignored @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. |
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 What remains is the annoyance of running tasks like 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 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. |
@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) |
Generally best to use a runtime factory, if such a runtime decision is needed 🤔 What sometimes happens is:
These are supposed to not happen during build time at all. Perhaps what's needed is preventing usage of In practice, if I do following, and the bundle did NOT explicitly declare
Then what I could get as useful feedback is a This could potentially allow for more aggressive inlining too. |
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. |
@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 |
@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 |
@stof but this doesn't work when the value is given from a bundle as value to a $resolvingBag = $container->getParameterBag();
$configs = $resolvingBag->resolveValue($configs); it fails with |
I don't think that's true. The 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:
There are various problems with this approach:
Especially for the last reason, the Using static env vars by default would flatten the learning curve:
Much simpler IMO. |
@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 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. |
@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 Requiring all bundles that already support runtime variables today to add |
@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 MY_ENV_VAR=false my_config:
enabled: "%env(bool:MY_ENV_VAR)%" So I think it would better to define for which
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. |
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. |
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 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. More specifically:
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... |
IIUC that be
|
@ro0NL thx, but those are not available in all Smfony versions I am using (currently migrating one app from 2.8 to 3.4...) |
@gggeek 2.8 does not support env variables at all... |
@stof yep. Otoh 3 does, and that's where the story about parameters started to get very complicated (imho of course) |
Thank you for this issue. |
@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.
A simple approach that (mostly) fixes the issue without reinventing the way that bundles process configuration. |
I would consider the reverse route instead, thus a |
@dkarlovi why it is not recommended it is still possible by doing We @sulu are using it to check if ghostscript executable is correctly installed. 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 |
@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: |
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. |
I found this interesting and gave it a try. My approach:
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. I tested that with a service where I injected Looks like it's more complex than I initially thought 😄 |
When an env var is dynamic, the host is the part that knows about it. With this in mind, using prefixes as discussed above ( 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 Then we patch the default recipe to set |
@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
|
are we really sure this is the way, compared to just using static |
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.
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 As mentioned in the PR:
|
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. |
There's another logical mistake with the "prefix" proposals BTW: they put the static/dynamic info on the reference (
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.
You're ignoring the full discussion, unless I didn't understand it 🤷 So yes, that's my best proposal :)
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. |
but why actually? what's wrong with explicit 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. |
@ro0NL you're missing the discovery part of env vars. |
@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 Of course, making that very clear in the docs will help to some degree. |
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. |
@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. |
@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. 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 Stack trace:
Is this an unintended side effect of MergeExtensionConfigurationPass after all 🤔 |
@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
An additional place could be
Finally, a similar comment could be added to |
@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 :) |
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 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. |
Whatever the chosen solution is, it would hopefully help with this scenario: 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. |
Hello all Is there any update on this? Is anyone working on a solution/solution design or is there an existing blocker? Thanks |
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:
A quick research shows up several bug reports of this kind:
Benefits of env vars
Env vars as used in Symfony provide two benefits:
.env
,.env.local
etc. Those are 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 ifstatic_env_vars
is set totrue
.In a later major release, I propose to flip
static_env_vars
totrue
by default to improve DX.Example
Benefits
.env
) rather than two (.env
andparameters.yaml
) for deployment specific configuration.env.local
(not easily achievable withparameters.yaml
)The text was updated successfully, but these errors were encountered: