Skip to content

[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

Closed
Pierstoval opened this issue Jan 5, 2018 · 53 comments
Closed

[RFC][Dotenv] Add a convenient way to retrieve env vars #25693

Pierstoval opened this issue Jan 5, 2018 · 53 comments

Comments

@Pierstoval
Copy link
Contributor

Pierstoval commented Jan 5, 2018

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 3.x, 4.x

We can access env vars in several ways, and the Dotenv class is already capable of handling env vars in all possible ways:

foreach ($values as $name => $value) {
$notHttpName = 0 !== strpos($name, 'HTTP_');
// don't check existence with getenv() because of thread safety issues
if (!isset($loadedVars[$name]) && (isset($_ENV[$name]) || (isset($_SERVER[$name]) && $notHttpName))) {
continue;
}

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 the populate() method.

Proposal

I propose to add Dotenv::get($name) and Dotenv::exists($name) methods that would retrieve env vars based on their existence in these cases and this order:

  • First in $_ENV
  • Then in $_SERVER
  • And fallback to getenv() even if it's not thread-safe

This 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:

if (!Dotenv::exists('APP_ENV')) {
    (new Dotenv())->load(__DIR__.'/../.env');
}

WDYT?

@xabbuh
Copy link
Member

xabbuh commented Jan 5, 2018

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 .env files.

@nicolas-grekas
Copy link
Member

same as @xabbuh
just use "%env(FOO)%" and free your services from knowing anything about where their configuration comes from.
bootstraping files are very different and need to be standalone.

@Pierstoval
Copy link
Contributor Author

I updated my proposal because you're right about dev&prod and env vars elsewhere: that's why a Dotenv::exists would also have to be mandatory

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Jan 5, 2018

@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 APP_ENV, which is absolutely mandatory if you want a working app. And this var cannot be solved by the DI, so we need something to make it available clearly and avoid PHP flaws that set the env vars in different places and make getenv not safe.

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');

@ostrolucky
Copy link
Contributor

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
#25523
#25162
#24686
#24113
#23949
#23899

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.

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Jan 7, 2018

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 Env component can be created. Then, Dotenv would require it, but all other Symfony components would possibly require it too if they load env vars.

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 Console component, like Composer or lots of others).

@nicolas-grekas
Copy link
Member

This would add another dependency to your app for something that is better done via parameters.
👎 on my side.
At last, inject parameter_bag if you want.

@Pierstoval
Copy link
Contributor Author

The problem is that the parameters part is only for the DI (even though DI is a huge part of the framework). Improving the Dotenv component might make sure we don't have issues like the ones recently spotted in the framework's recipes.

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 🤔

@nicolas-grekas
Copy link
Member

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.

@Pierstoval
Copy link
Contributor Author

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?

@nicolas-grekas
Copy link
Member

use $_SERVER['FOO'], or services if you have the container somehow

@Wirone
Copy link
Contributor

Wirone commented Jun 11, 2018

@nicolas-grekas We're encountering issues related to $_SERVER / $_ENV. We're running Docker container based on php:7.2-apache image with default configuration and passing env variables through environment option in docker-compose.yml.

Our entry script does not see APP_ENV within $_SERVER, but it's populated into $_ENV.

@Saphyel
Copy link
Contributor

Saphyel commented Jun 11, 2018

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.
Maybe this proposal should go to PHP to request update/deprecate _SERVER and/or _ENV or do something about getenv/setenv to make it easier to use/standard.

@fugi
Copy link

fugi commented Jul 27, 2018

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:

// don't check existence with getenv() because of thread safety issues
            if (!isset($loadedVars[$name]) && (isset($_ENV[$name]) || (isset($_SERVER[$name]) && $notHttpName))) {

but it is still used in EnvVarProcessor:

} elseif (isset($_ENV[$name])) {
            $env = $_ENV[$name];
        } elseif (isset($_SERVER[$name]) && 0 !== strpos($name, 'HTTP_')) {
            $env = $_SERVER[$name];
        } elseif (false === ($env = getenv($name)) || null === $env) { // null is a possible value because of thread safety issue

and there is only $_SERVER in index.php:

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

@Pierstoval
Copy link
Contributor Author

As you can see, the EnvVarProcessor usage is only a fallback if $_ENV and $_SERVER are not set, but in most cases, they should be set, even though it's not sure depending on your software architecture.

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.)

@fugi
Copy link

fugi commented Jul 30, 2018

Yeah, I totally agree. Like you wrote, it should be:

First in $_ENV
Then in $_SERVER
And fallback to getenv() even if it's not thread-safe

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.

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Sep 11, 2018

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 $_ENV but not $_SERVER, which makes it load default .env file.

Related issue: symfony/panther#68

The only solution is to make index.php fall back on $_ENV, so either we continue add (or propose to add) a certain amount of dirty code in our front controller just to fit to the problems created by PHP's native inconsistencies, or we add much prettier Env::has() and Env::get() or any similar feature to the framework... And I'd be happy to do it if the core team agrees.

@bocharsky-bw
Copy link
Contributor

bocharsky-bw commented Sep 15, 2018

I agree with @Pierstoval , index.php cannot be based on $_SERVER only, we have to consider $_ENV at least. I also described a problem in symfony/recipes#461 (comment) - if you set up some env vars and run built-in Symfony web server - index.php does not have env vars in $_SERVER, but has in $_ENV. So if we made it possible to consider both $_SERVER and $_ENV in index.php it would be possible to have Symfony web server registered in bundles.php for dev mode only but load the website in any mode you want: prod, test, etc. - that would be cool and easy! I won't need to create a additional front controllers then like index_prod.php, index_test.php, etc. and hardcoding APP_ENV.

And now the question is how to make it possible to consider env vars in both $_SERVER and $_ENV in index.php? I agree with @nicolas-grekas that index.php file should be thin and with minimum deps, but this way we'll have long conditions like:

if (!isset($_ENV['APP_ENV']) || !isset($_SERVER['APP_ENV'])) {
    // load .env file
}

$env = $_SERVER['APP_ENV'] ?? ($_ENV['APP_ENV'] ?? 'dev');

And that's only if we do not want to consider getenv(). So probably having Env::has() and Env::get() would help here and make code more readable at least. I'm not sure if %env(APP_ENV)% reads both $_SERVER and $_ENV to provide a fallback, but if it does - we can keep in sync %env()% and Env::get() then to be consistent.

@Pierstoval
Copy link
Contributor Author

I'm not sure if %env(APP_ENV)% reads both $_SERVER and $_ENV to provide a fallback, but if it does - we can keep in sync %env()% and Env::get() then to be consistent.

Yes, the DI component takes this in account, but only the DI component.

@bocharsky-bw
Copy link
Contributor

Great, so what about to take out that logic into a separate class e.g. Env as you suggested. Then we can reuse this Env in both places: for %env(APP_ENV)% and in index.php at least, i.e. share the same logic. Devs can still use low level $_SERVER/$_ENV in some cases if needed, but using Env::get() would have more sense for me, as we can see we should take into account both $_SERVER/$_ENV. So I like this idea.

@Wirone
Copy link
Contributor

Wirone commented Sep 24, 2018

Would be good if Env::get() could cast results, using the same logic as EnvVarProcessor uses.

So with signature like:

class Env {
    public function get(string $name, string $type = 'string', $defaultValue = null) {
    }
}

we could achieve many useful things, like retrieving boolean configuration flags with true as default value - simple getenv requires "complex" conditions for checking if variable is set, casting it to bool with filter_var and so on, which is useless noise in the code since it could be reusable.

// Examples
Env::get('FORCE_SSL', 'bool', true);
Env::get('ALLOWED_REMOTE_IPS', 'json', []); // when you want to resctrict usage directly in front controller

As default, Env::get() would act like simple string-based read-as-is common way for accessing environment variables.

@Pierstoval
Copy link
Contributor Author

@Wirone your proposal is interesting, but I would even go further by adding the complete EnvVarProcessor feature so we could do something like Env::get('json:base64:file:CREDENTIALS_FILE'), but that's maybe for another time. First, I'd like this RFC to be approved before moving to further ideas, which is not the case now :)

@Pierstoval Pierstoval changed the title [Dotenv] Add a convenient way to retrieve env vars [RFC][Dotenv] Add a convenient way to retrieve env vars Sep 24, 2018
@Wirone
Copy link
Contributor

Wirone commented Sep 24, 2018

@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).

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 24, 2018

Env::get('json:base64:file:CREDENTIALS_FILE')

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.

@Wirone
Copy link
Contributor

Wirone commented Sep 24, 2018

@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, DotEnv can be used outside of Symfony and its DI (we use it in Yii1 application), it would be really good if component could solve issues from real world.

What's the problem with extracting EnvVarProcessor code related to parsing variables to some EnvReader or something and use it in processor along with static methods in Env? It won't break anything, only add new possibilities.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 24, 2018

There is no logic in EnvVarProcessor: it just maps a string to eg file_get_contents. That doesn't make a component: use file_get_contents instead, etc. The EnvVarProcessorInterface is only useful in the context of DI, to allow a pluggable config system. If you're not using it, just use the PHP function directly.

@Wirone
Copy link
Contributor

Wirone commented Sep 24, 2018

@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 EnvVarProcessor. But logic could be used elsewhere, for loading standardized environment variables outside DI.

@nicolas-grekas
Copy link
Member

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)

@Pierstoval
Copy link
Contributor Author

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 Env::get() and Env::has() would solve this. Still against the idea, even when considering the uncountable amount of issues regarding env vars in PHP?

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... 😕

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 24, 2018

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.

@Wirone
Copy link
Contributor

Wirone commented Sep 24, 2018

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 ConfigDataCollector supports them out of the box in constructor. And this is only because I've read the code, it's not documented. This is not what people expect. Developers want to use tools to build applications and focus on business logic, not repeating themselves in several projects or making meta-bundles just for reusing code which should be part of the framework. I don't get why you don't understand the purpose of common env vars loader, but since \Throwable (for exception interface/marker) that had to be explained several times, probably nothing will surprise me.. There are real-life examples of usage that goes beyond Symfony Framework and if Symfony as a brand want to "buy" people, it should think about it and don't focus on geeks who find it enjoyable to write multiple lines of code to achieve simple things.

Sorry for the rant, but I'm really disappointed.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 25, 2018

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!
And in the context of Symfony full stack, the DI configuration already allows accessing env vars so this wouldn't be used nor recommended.
But I'm going to step down from this issue, I'm exposing myself too much to emotional comments by opposing. My arguments are written, I'll let others chime in now. I'm fine with the final decision whatever it is. What matters is having a discussion.

@Wirone
Copy link
Contributor

Wirone commented Sep 25, 2018

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 EnvVarsProcessor (and by that I mean ability to process variables and cast them to expected types, additionally with support for chaining, like json:base64:SOME_ENCODED_JSON) and share it between DI and DotEnv component would solve these issues. Component's name would be slightly misleading then, but if you think about "DotEnv" as a trademark, not as meaningful name, it could be library for handling ENV variables - from .env files, but also in general.

@linaori
Copy link
Contributor

linaori commented Sep 25, 2018

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 propose to add Dotenv::get($name) and Dotenv::exists($name) methods that would retrieve env vars based on their existence in these cases and this order [...]

I don't see harm in adding those helper functions to the Dotenv class. They will make it easier for developers to get env vars, while it doesn't add downsides. It reduces the need to write helper functions while not bringing harm to any of the existing code.

The counter arguments:

xabbuh: this would mean that you would always have to install the Dotenv component.

The only reason you don't have to install it, is because if the APP_ENV variable is set, it won't hit the code calling Dotenv. While I do not recommend calling the parsing in production code, it brings no harm to have the package present, it's small and contained. If someone adds it to require instead of require-dev, no real harm is done.

nicolas-grekas: use "%env(FOO)%" and free your services from knowing anything about where their configuration comes from. bootstraping files are very different and need to be standalone.

I agree that these are separate concerns and should therefore not both be in the Dotenv class. However, due to the nature of them being static calls, adding the functions would be a quick win, while deprecating them and calling them from a different class would be a piece of cake. The other alternative would be to add a new class in the component that is dedicated to the env var get/has(/set).

Saphyel: 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. Maybe this proposal should go to PHP to request update/deprecate _SERVER and/or _ENV or do something about getenv/setenv to make it easier to use/standard.

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->getEnvVar('foo') in the abstract controller, they are just helper functions to make a dev's life easier.

This RFC gets a 👍 from me (for as much as that counts).

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Sep 25, 2018

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 Symfony\Component\Env component.

It would contain 2 classes: Env (RFC proposal) and EnvFile (renaming of the initial DotEnv). This would deprecate symfony/dotenv as component, and recommend symfony/env and put it in the require section instead of require-dev

No real change needed for EnvFile, except that it can use the proposed Env class feature to get & set env vars.

And Env would have 3 static methods:

  • Env::has(string $varName): bool
  • Env::get(string $varName): mixed
  • Env::set(string $varName, mixed $value): void

Few changes in Symfony recipes, for bin/console (command-line starter), public/index.php (HTTP front controller) and the proposed bootstrap.php that would set up the project (or only the test env for phpunit).

They would use Env::has('APP_ENV') instead of the dirty hacks, and execute EnvFile::load(__DIR__.'/../.env') if it returns false.

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.

@linaori
Copy link
Contributor

linaori commented Sep 25, 2018

I'm not sure if renaming the component is worth it, why not add the Env class to the existing component?

@ogizanagi
Copy link
Contributor

xabbuh: this would mean that you would always have to install the Dotenv component.

iltar: The only reason you don't have to install it, is because if the APP_ENV variable is set, it won't hit the code calling Dotenv. While I do not recommend calling the parsing in production code, it brings no harm to have the package present, it's small and contained. If someone adds it to require instead of require-dev, no real harm is done.

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 APP_ENV being set or not).
Then in production, it might happen an env var is missing but would fallback to the dev value in .env file without notice. IMHO, that's an argument to make it a different component.

@linaori
Copy link
Contributor

linaori commented Sep 25, 2018

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

@ogizanagi
Copy link
Contributor

@iltar : That's exactly what I mean. DO NOT install the Dotenv component in production.
That's why the Env::get/has/set implem should live in another component.

@Pierstoval
Copy link
Contributor Author

That's why the Env::get/has/set implem should live in another component.

Then @ogizanagi wdyt about this comment?

@linaori
Copy link
Contributor

linaori commented Sep 25, 2018

@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 require because you forgot --dev in your composer, it breaks your production env.

@ogizanagi
Copy link
Contributor

ogizanagi commented Sep 25, 2018

@Pierstoval : In your proposal, you suggest to deprecate the Dotenv component in favor of a Env component containing both the Env class and env parsing from a file. So that's the same issue.

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.

@Pierstoval
Copy link
Contributor Author

In my proposal, we keep DotEnv almost as-is, just renaming it for the component name's consistency, bringing it into a better symfony/env component, more suitable to me than having both symfony/env AND symfony/dotenv 😑

@ogizanagi
Copy link
Contributor

@Pierstoval : But you may need Env in production while you want to avoid having EnvFile in this very same environment for reasons expressed in #25693 (comment). So that should be 2 distinct components, otherwise it doesn't change anything. Am I misunderstanding you ? 🤔

@ostrolucky
Copy link
Contributor

@Pierstoval Your proposal #25693 (comment) doesn't mention renaming DotEnv, only about creating new Env component. Or am I reading it wrong?

I agree with other guys that having to include current DotEnv is not optimal.

@Pierstoval
Copy link
Contributor Author

@Pierstoval Your proposal #25693 (comment) doesn't mention renaming DotEnv, only about creating new Env component. Or am I reading it wrong?

You probably misread:

It would contain 2 classes: Env (RFC proposal) and EnvFile (renaming of the initial DotEnv).

But maybe I wasn't clear enough 😉

@Pierstoval : But you may need Env in production while you want to avoid having EnvFile in this very same environment for reasons expressed in #25693 (comment). So that should be 2 distinct components, otherwise it doesn't change anything. Am I misunderstanding you ?

I don't see the advantage of having two components to handle env vars. In my second proposal, DotEnv is just moved to another more global component related to env. Because in fine, DotEnv would need to fetch & set env vars by using the proposed Env class methods. No need for two symfony/dotenv and symfony/env components for this IMO.

And yes, this would require the symfony/env component to be installed in the require section, and not in require-dev. Checks about DotEnv existing or not would be useless, especially if we can just use if (!Env::has('APP_ENV')) { /* load env file */ }.

@ogizanagi
Copy link
Contributor

And yes, this would require the symfony/env component to be installed in the require section, and not in require-dev. Checks about DotEnv existing or not would be useless, especially if we can just use if (!Env::has('APP_ENV')) { /* load env file */ }.

Then it'll be incompatible with symfony/recipes#466 changes. That's why I suggested keeping two components.

@Pierstoval
Copy link
Contributor Author

symfony/recipes#466 is not merged yet, therefore it's still possible to make both features compatible :)

@Wirone
Copy link
Contributor

Wirone commented Sep 26, 2018

I am sure this will be helpful, however I agree that DotEnv::get() would be misleading.

My 2 cents:

  • DotEnv class should be used only for handling env files
  • Env variables should be checked/read via another class, e.g. Env (obviously)
  • As I said, if we consider DotEnv (component) as "trademark", not meaningful name, it could contain both classes related to env without changing its name BUT only if DotEnv component will be used in prod as in Allow to override .env files for specific environments recipes#466 (regular Composer require, not dev). Otherwise I agree that new component is required (Env loaded in all environment, DotEnv only in dev)
  • Nice to have: Env::get() could support processing values like DI's EnvVarProcessor does
  • I also think that load order ($_SERVER, $_ENV, getenv) can be configurable with static method, which would validate if only supported sources are provided when invoking. Something like Env::setSources(['server', 'env', 'getenv']);, while array argument could be changed to ...string $source so usage would be Env::setSources('server', 'env', 'getenv'). Maybe there should be also dotenv source, supported only when DotEnv component is installed. So basically, without calling Env::setSources() it would used default loading order, but component will allow user to customize how it works in their application.

@Pierstoval
Copy link
Contributor Author

@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 😄

@nicolas-grekas
Copy link
Member

Closing as this won't get approved.

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

No branches or pull requests

10 participants