-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[RFC][Dotenv] Add a convenient way to retrieve env vars #25693
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
I am not sure this would really solve such issues. Because this would mean that you would always have to install the Dotenv component. Otherwise, your front controller's code would lead to errors in environments where you set the env vars through your vhost settings or anywhere else outside |
same as @xabbuh |
I updated my proposal because you're right about dev&prod and env vars elsewhere: that's why a |
@nicolas-grekas It's not only about the DI component and the services, it's also about setting up the project in the front controllers. Today, environment is based on I don't think having this is convenient in our front controllers: if (!isset($_SERVER['APP_ENV'], $_ENV['APP_ENV']) && false === getenv('APP_ENV')) {
// Load .env
}
$env = $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? getenv('APP_ENV'); |
Ah yes, situation regarding $_SERVER/$_ENV/getenv()/putenv() is a mess. There were already ton of bugs based on confusion between these and more will come. #25559 That said, @nicolas-grekas suggeestion to avoid using it directly and just retrieve it via DI is good one. Your code example isn't so bad either. Come on, it's just one place, isn't it? If you have a project with multiple front controllers, just create simple function for it. However, simple composer package for just doing that would be handy. It could be used in all symfony components as an abstraction layer for reading/writing env vars to avoid problems such as the ones I linked. |
We could get inspiration from components like oscarotero/env or vlucas/phpdotenv. This is "just another env library" like it is right now. Maybe another Like @ostrolucky linked, there are a lot of issues regarding environment vars. And there also are issues for external components (like the front controllers and console, or maybe CLI applications using the |
This would add another dependency to your app for something that is better done via parameters. |
The problem is that the parameters part is only for the DI (even though DI is a huge part of the framework). Improving the There's also some dichotomy depending on whether you're using php in command-line, with apache + mod_php, with nginx + php-fpm... Considering the high diversity of projects (docker, dedicated, PaaS), and the fact that PHP doesn't really have a stable environment vars management, I still think we should consider giving Symfony devs a proper way of retrieving env vars instead of relying on hacks like in the front controller or in the console file 🤔 |
The front controller is very special as it is almost "naked", we don't have nor want any kind of dependencies there. The Dotenv component is not a service, it's only for bootstrapping, and we don't want to make it a service. Then, in your own code in your apps, you have DI at hand. |
So we don't take in account any project not under Flex & FrameworkBundle? What about the console? Does the console really need the DI if it wants to use env vars? |
use |
@nicolas-grekas We're encountering issues related to Our entry script does not see |
I disagree on this. Dotenv is a dev tool for people that doesn't know how to use the env. vars, If you use it everywhere becomes a production tool and I don't think we need this tbh. |
It's the same with us. We are using Docker and set enviroment variables to configure application on different environment(devs/prod). And there is inconsistency in Symfony libraries in some cases you are using getenv but in some it is removed because of thread safety issues. But this is simple if you want to be thread safety use $_SERVER to configure your application, why remove getenv from Dotenv.php:
but it is still used in EnvVarProcessor:
and there is only $_SERVER in index.php:
|
As you can see, the EnvVarProcessor usage is only a fallback if I still think that having a proper way to retrieve & check existence for env vars would be useful for projects not using the DI component (especially third-party ones, other frameworks or CMS built with some Symfony components, etc.) |
Yeah, I totally agree. Like you wrote, it should be:
Why omitting fallback to getenv? In Docker, by default, variables are passed by environment variables. If you want to pass it to PHP script you need to write additional ENTRYPOINT script and change Apache vhost config to set $_SERVER. |
I found another use-case where env can cause problems. When using Symfony/Panther, it will start a PHP server by using Symfony/Process. By default, it sends current's process environment vars to the process, but the PHP built-in server sets Related issue: symfony/panther#68 The only solution is to make |
I agree with @Pierstoval , index.php cannot be based on And now the question is how to make it possible to consider env vars in both
And that's only if we do not want to consider |
Yes, the DI component takes this in account, but only the DI component. |
Great, so what about to take out that logic into a separate class e.g. |
Would be good if So with signature like:
we could achieve many useful things, like retrieving boolean configuration flags with
As default, |
@Wirone your proposal is interesting, but I would even go further by adding the complete EnvVarProcessor feature so we could do something like |
@Pierstoval yeah, that approach also would be really helpful. Also it would be more consistent with DI. I'm 👍 for it (but of course with support for default value). |
that's not proper OOP: env processors are services. But making this work with static methods means they should access some global state. Instead, you should decouple your code and not care where the injected value comes from - env or not. That's what we use DI for. Still 👎 on my side. |
@nicolas-grekas aren't Symfony components meant as standalone libraries that solve some specific scenarios, then bound together they create framework? And that's the case, What's the problem with extracting |
There is no logic in EnvVarProcessor: it just maps a string to eg |
@nicolas-grekas how is it "no logic"? All the logic for processing common types (e.g. bool, json, base64, int, float) could be moved to DotEnv component and re-used in DependencyInjection's |
There is almost zero business logic in this code yes: that's just mapping strings to functions. The mapping itself needs logic, but not the "function". Using PHP directly is way more effective (except in the context of DI) |
Apart for the env processor (which I would have preferred to discuss in another issue), I agree with @Wirone about this: Symfony components are meant to be standalone libraries that solve common programming issues. The issue is the following: PHP handles environment vars in an inconsistent way. Implementing I mean, Symfony is a top framework, and I personnally think it would be really sad that it does not have a simple implementation that fixes a decade-old PHP issue... 😕 |
Yep, still not sold. There is a ton of function helpers that would be useful. Mine would be ini_get_bool(). But a collection of (random) helpers doesn't make a component. |
And, sadly, this is why people choose other frameworks. They want getting things done in a simple way. I like Symfony because of relatively clean architecture, it's flexible and magic in a reasonable manner. But today I've learned that to display application's name and version on the debug toolbar I need to create compiler pass which will add arguments to service defined without them, even if Sorry for the rant, but I'm really disappointed. |
Don't be, this is just code. Symfony is not exclusive to any non-Symfony packages, quite the contrary. If someone wants to use or create one that provides these helpers, that's perfectly fine! |
To be clear: I know DI supports loading ENV vars, but here we're talking about use cases where DI container is not (yet) available (e.g. front controller, but not only) and common way for loading and normalizing ENV vars would be helpful. Issues are with Docker, PHP built-in server - these are real world cases that have to be solved with custom code, which is possible, but is it necessary? Moving logic from |
I agree with most arguments written here. From the perspective of Symfony, the DI component already handles ENV vars cleanly. When not using the DI component, you'll be stuck writing your own helper functions for env vars, not just get/exist, but even put from time to time. I've had to do this on more than one occasion. I usually have Symfony present, but I'm not always initializing Symfony at every point. Sometimes using the DI for tiny things is also over-kill. What are the downsides when adding these 2 (or 3 if you add set/put) to Dotenv? From nicolas' comments, I see it's easier to to avoid clean architecture and that helper functions do not easily justify their own component. The proposal:
I don't see harm in adding those helper functions to the The counter arguments:
The only reason you don't have to install it, is because if the
I agree that these are separate concerns and should therefore not both be in the
This is not a dev tool for people that don't know how to use env vars, this tool is designed to load default env vars for a development environment. I do agree that the get/has(/set) should be solved in PHP, but that won't solve this problem and if anything, will lead to a polyfill for 7.4, which is still far away. I don't think processors should be taken along, it should only work with raw values as it's a helper function to get the value, not process it. I don't see harm in adding these tiny features from a DX perspective, it will help a bunch of developers while not providing any downsides. It's not like we're adding This RFC gets a 👍 from me (for as much as that counts). |
Should I rephrase the RFC to look for other kind of approvals? Situation: Currently, PHP handles env vars inconsistently. Proposal: Create dedicated tools in Symfony to fix this. Received counter-argument: Use the DI component. Env vars are indeed used in the DI Component, but not every project using Symfony components use the DI component, so we can't just say "If you want clean env vars, use DI". That's not just how we solve problems for front controllers or from-scratch projects or other frameworks or CMSes etc. So, let's change the initial RFC. Let's create a simple It would contain 2 classes: No real change needed for And
Few changes in Symfony recipes, for They would use This is a diff as an example of what would change in the front controller: symfony/recipes@master...Pierstoval:patch-1 The advantage is that ANYONE could use these features in ANY project in ANY context (test, http, CLI...) and it would JUST WORK. No dirty hack, no necessary knowledge of the fact that PHP doesn't know how to properly use env vars. |
I'm not sure if renaming the component is worth it, why not add the |
It does harm. With planned changes in symfony/recipes#466, having the Dotenv component installed means we'll try to load the env files (independently of the |
That sounds like a terrible idea. If my production forgets to set an env, I rather have it 500 than connect to the wrong database |
@iltar : That's exactly what I mean. DO NOT install the Dotenv component in production. |
Then @ogizanagi wdyt about this comment? |
@ogizanagi I'm referring to that PR, not this request. It's far too dangerous to just assume you should load .env code based on availability of a package. If for some silly reason you move this package to |
@Pierstoval : In your proposal, you suggest to deprecate the Dotenv component in favor of a About the RFC itself, I understand the need and it doesn't seem a burden for Symfony to maintain it. No stronger opinion than that actually. |
In my proposal, we keep DotEnv almost as-is, just renaming it for the component name's consistency, bringing it into a better |
@Pierstoval : But you may need |
@Pierstoval Your proposal #25693 (comment) doesn't mention renaming DotEnv, only about creating new I agree with other guys that having to include current DotEnv is not optimal. |
You probably misread:
But maybe I wasn't clear enough 😉
I don't see the advantage of having two components to handle env vars. In my second proposal, And yes, this would require the |
Then it'll be incompatible with symfony/recipes#466 changes. That's why I suggested keeping two components. |
symfony/recipes#466 is not merged yet, therefore it's still possible to make both features compatible :) |
I am sure this will be helpful, however I agree that My 2 cents:
|
@Wirone I agree with your 3 first points, but not with adding env processing & configurable order, and I would postpone discussions about this, putting them in other issues 😄 |
Closing as this won't get approved. |
We can access env vars in several ways, and the
Dotenv
class is already capable of handling env vars in all possible ways:symfony/src/Symfony/Component/Dotenv/Dotenv.php
Lines 72 to 77 in f95ac4f
There are also some cases where the current Flex recipes are using
$_SERVER
and it seems it breaks and should use$_ENV
.A nice example is the symfony/recipes#331 issue: the
APP_ENV
env var is created before the entrypoint is executed, and PHP doesn't set it in$_SERVER
but sets it in$_ENV
.It's possible that it's a PHP issue, but I think we can handle this right in
DotEnv
, and possibly the same way it handles checks in thepopulate()
method.Proposal
I propose to add
Dotenv::get($name)
andDotenv::exists($name)
methods that would retrieve env vars based on their existence in these cases and this order:$_ENV
$_SERVER
getenv()
even if it's not thread-safeThis way, we could use this in our front-controllers, either console or web, and we would never expect any edge case from this.
This would mean that Dotenv would be required in dev & prod, and not only in dev, but then the APP_ENV check could be reduced to this:
WDYT?
The text was updated successfully, but these errors were encountered: