Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect and convert types #104

Closed
oscarotero opened this issue Jul 15, 2015 · 38 comments
Closed

Detect and convert types #104

oscarotero opened this issue Jul 15, 2015 · 38 comments

Comments

@oscarotero
Copy link

In my .env file I have values like:

APP_DEBUG=true
APP_DEV=false

But the type of these values is always string:

var_dump(getenv('APP_DEBUG')); // string(4) "true"
var_dump(getenv('APP_DEV')); // string(5) "false"

I think the values like true, false, null (or even numbers), without quotes, should be converted to the correct type (boolean, integers, etc), because in cases like this, APP_DEV returns "false" so it's evaluated as true:

if (getenv('APP_DEV')) {
    // do something
}
@GrahamCampbell
Copy link
Collaborator

We do this don't we?

@GrahamCampbell
Copy link
Collaborator

We have tests that prove this works.

@oscarotero
Copy link
Author

Currently it returns false if the variable is not defined, but I don't get the values true or false converted to booleans.
In the tests I can see this fixture and the expected value is string, not boolean.

@GrahamCampbell
Copy link
Collaborator

What version?

@GrahamCampbell
Copy link
Collaborator

Hmmm.

@oscarotero
Copy link
Author

The latest dev version (2.1@dev)

@GrahamCampbell
Copy link
Collaborator

NB, 2.0.x is the latest version. I don't recommend using an unreleased version as it's still subject to change.

@oscarotero
Copy link
Author

Yes, you're right 😄 2.0.1 does the same.

@vlucas
Copy link
Owner

vlucas commented Jul 16, 2015

@GrahamCampbell These are converted in Laravel, but not in phpdotenv itself. The only reason this is not done in phpdotenv is because I originally tried to keep it as close to shell as possible, and in PHP ENV variables, everything is a string. I am not opposed to adding this functionality in, though - and we can probably even just grab the code from Laravel that does this and put it in phpdotenv.

@oscarotero
Copy link
Author

Maybe, a way to keep backward compatibility is make this behaviour optional. For example:

$dotenv->load(); // do not convert types
$dotenv->load(true); // convert types

@oscarotero
Copy link
Author

Here's how laravel handle this: https://github.com/laravel/framework/blob/8c614010648bffb8bc24c3b532db08619e9a5983/src/Illuminate/Foundation/helpers.php#L626

@GrahamCampbell
Copy link
Collaborator

Yeh, but laravel makes it impossible to have null as a string, which isn't ideal.

@oscarotero
Copy link
Author

Yes, I think this should be handled when loading values, so it's possible to differentiate between null and "null".

@oscarotero
Copy link
Author

Well, thinking about this, maybe this is not good idea and it's better the way laravel handles this, using a custom function to consume the environment variables. So if you want to convert the values, use the custom function and if you don't, use getenv.

var_dump(env('MY_BOOLEAN_VALUE')); // bool(true)
var_dump(getenv('MY_BOOLEAN_VALUE')); // string(4) "true"

@vlucas
Copy link
Owner

vlucas commented Jul 21, 2015

So if you want to convert the values, use the custom function and if you don't, use getenv

This approach is above the level of phpdotenv, since it does not provide any convenience methods by itself. To me, this is an argument supporting keeping things they way they are.

@GrahamCampbell
Copy link
Collaborator

Yeh, we probably don't need to change anything tbh.

@ghost
Copy link

ghost commented Jul 26, 2015

I've been wanting something like this, but probably alongside the current allowedValues or required. basically setting a schema of acceptable variables and their values

@barbushin
Copy link

Same problem #129

@vlucas
Copy link
Owner

vlucas commented Oct 28, 2015

There is a solution in #121. We need to get this added to the README.

@jpcercal
Copy link

jpcercal commented Nov 4, 2015

@vlucas thanks guy, i am happy to help.

@natsu90
Copy link

natsu90 commented Mar 23, 2016

I have to use following solution;

filter_var(getenv('DB_DEBUG'), FILTER_VALIDATE_BOOLEAN)

refs: http://stackoverflow.com/a/10645030

@GrahamCampbell
Copy link
Collaborator

Leaving this for people to wrap.

pH-7 added a commit to pH-7/Link2Payment that referenced this issue Nov 7, 2017
@drmax24
Copy link

drmax24 commented May 21, 2018

such constructions will kill php in a meantime

filter_var(getenv('APP_DEBUG'), FILTER_VALIDATE_BOOLEAN)

Instead of executing the simple command as in a normal language you start to add useless sophistication

"very simple" filter_var(getenv('APP_DEBUG'), FILTER_VALIDATE_BOOLEAN)
No junior guy can write this from his head and senior guys are senior just because they keep in mind this. So senior php programmer is senior because he stores 80% of the useless information in his mind. This is where you lead this platform with your popular package.

So simple and elegant! Every php line should be like this! Keep up good work!

$kernel = new AppKernel(
    filter_var(getenv('APP_ENV')) ?? 'dev',
    filter_var(getenv('APP_DEBUG'), FILTER_VALIDATE_BOOLEAN) ?? false
);

My comments to this code would be:
Php is doomed

@bs-thomas
Copy link

@drmax24 I agree with you.

What got me here is because of this problem. Actually I must say I do not quite agree on automated conversions inside dotenv file.

I use Laravel, and including phinx for the independent library for database migrations. There is a problem caused indirectly because of this mindset.

cakephp/cakephp#12669

@patricknelson
Copy link

patricknelson commented Nov 2, 2019

Environment variables really don't work quite the same way as most typical languages when it comes to booleans, since they don't really exist in the traditional sense. Personally, I'd shoot for broader compatibility and just embrace how environment variables work.

An elegant way to do this that "just works" even if you're not using dotenv would be to also leverage PHP's type juggling abilities, and simply avoid using true or false in your environment variables entirely, like so:

# False:
MY_VAR=0
MY_VAR=
# ... or just don't define it...

... then...

if ((int) getenv('MY_VAR')) {
    // ... won't run.
} else {
    // ... always runs.
}

In every scenario above, (int) getenv('MY_VAR') will always cast to 0. I know I'll be using this method since I'm also dockerizing/containerizing/kubernetes-er-izing my application, so .env won't exist and this library can't be used anymore anyway. Just thought I'd throw this technique out there as an alternative suggestion!

@llbbl
Copy link

llbbl commented Apr 13, 2021

So where did we land on this. If this isn't going to be a planned feature can we at least update README to explicitly say that booleans must be handled in user code.

@GrahamCampbell
Copy link
Collaborator

https://www.php.net/manual/en/function.getenv.php has return type string|false. One cannot do type conversion and still call what you are doing environment variables.

@llbbl
Copy link

llbbl commented Apr 13, 2021

@GrahamCampbell do you happen to know where the env function ended up in Laravel 8+ ? Are you open to changing the language on README that talks about getenv and if the user even cares about running php with thread safety (mod Apache). Maybe a link to a SO that explains thread safety etc, would probably help a lot of people.

@GrahamCampbell
Copy link
Collaborator

Yes, the env function source code is: https://github.com/laravel/framework/blob/8.x/src/Illuminate/Support/Env.php.

@llbbl
Copy link

llbbl commented Apr 13, 2021

@GrahamCampbell Thanks for the link! Any thoughts on the other question? Happy to take a crack at clearing up the Readme. I don't want to waste time unless you are open to it.

@thexpand
Copy link

thexpand commented May 26, 2021

And this issue is closed, because?

EDIT: Sorry, I missed @GrahamCampbell's comment about the return type of getenv(). Now I get it. Thanks anyway :)

@llbbl
Copy link

llbbl commented May 26, 2021

And this issue is closed, because?

EDIT: Sorry, I missed @GrahamCampbell's comment about the return type of getenv(). Now I get it. Thanks anyway :)

vlucas sounded like he was not opposed to (at least optionally) replicating Laravel return types but Graham most definitely is. I tried pushing for updated Readme to at least explain their reasoning and hopefully remind people coming back to this library that it doesn't work how you are probably expecting.

@GrahamCampbell
Copy link
Collaborator

Environment variables are strings. The type of an environment variable repository is array<string, string>. Anything else is not doing "environment variables". It's some other kind out out of scope transformation.

wkhayrattee added a commit to wkhayrattee/WordPress-With-Composer that referenced this issue Jul 17, 2021
… default assign them boolean values.

So to remedy this, let's refrain from using any typical boolean values.
Reference: vlucas/phpdotenv#104
@rodurma
Copy link

rodurma commented Feb 24, 2022

You can just use

$active = $_ENV['SOME_CONFIG'] == 'true';

@patricknelson
Copy link

But you shouldn’t, that’d be crossing concerns and gets confusing. Just treat environment variables as strings 100% of the time, because that’s what they are. If you need a number or need a quick way to interpret an environment variable as a Boolean, the simplest thing to do is just type cast it to int. See my example above for a bit more explanation 😄

@CodeByZach
Copy link

CodeByZach commented Sep 16, 2023

Can someone explain to me why something as simple as the absence of quotations couldn't be used to indicate a non-string data type? I believe this, along with the ability to specify default values, would be a welcome improvement.

variable_1="apple" (string)
variable_2="true" (string)
variable_3=true (bool)
variable_4="24" (string)
variable_5=24 (int)

@patricknelson
Copy link

If you read the thread above, several of us give the explanation for this: Environment variables are strings.

Environment variables can come from several sources, not just .env. Not only that, but in each of those sources, including/excluding quotes at definition time doesn’t change their data type once accessed in those other hybrid environments (e.g. bash, docker, etc). Since phpdotenv is an abstraction layer that operates on top of this would likely need some method by which to do the same level of type interpretation across all forms of input, not just .env and I’m not sure that’s possible. For example, once it’s reached getenv(), $_ENV and $_SERVER, I’m not sure it’s possible to correctly infer the originally defined data type (as it was defined, i.e. with or without quotes, like your example). So, why treat .env differently? I assume this would cause unexpected behavior and introduce more bugs than benefits.

But that’s just my opinion on why this is probably not being done. 😊

@patricknelson
Copy link

That said… I could see phpdotenv possibly adding this as a feature (disabled by default) but it could only work in some limited situations since it does take these values from multiple sources, this would likely happen after variables coalesced together (unless this feature explicitly only applies to .env where we have access to the original definition and the developer has explicitly opted into interpreting missing quotes as a string/bool/etc).

For example, it could logically interpret ”false” (string) as false (bool) but it would be ambiguous to interpret ”” (empty string) as a bool representing false and would still need some custom application layer logic (which for example would have an understanding of the variable itself in order to make that decision, i.e. it already knows that particular var should be a bool).

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

No branches or pull requests