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

Don't overwrite existing environment variables. #17

Merged
merged 1 commit into from
Feb 15, 2014
Merged

Don't overwrite existing environment variables. #17

merged 1 commit into from
Feb 15, 2014

Conversation

pda
Copy link
Contributor

@pda pda commented Feb 4, 2014

Ruby's dotenv does this with ENV[key] ||= value.

The point of dotenv / phpdotenv is to enable twelve-factor style configuration
via environment, so it's important to be able to override these .env files
with external environment variables.

Ruby's dotenv does this with `ENV[key] ||= value`.

The point of dotenv / phpdotenv is to enable twelve-factor style
configuration via environment, so it's important to be able to
override these `.env` files with external environment variables.
@stanlemon
Copy link
Contributor

+1 I think this is a good addition. For BC maybe it would make sense to offer some sort of parameter to control this?

@pda
Copy link
Contributor Author

pda commented Feb 5, 2014

I'd suggest “backwards compatibility” be handled through versioning.

This should be tagged as v2.0.0 to represent the backwards-incompatible change.
Anybody is free to stick to v1.x.x (currently v1.0.6) if they depend on the old broken behavior.

This keeps the complexity in version history instead of runtime code.

Edit: http://semver.org/ for reference.

@stanlemon
Copy link
Contributor

My point was that the change in behavior could be made w/o having to break the rules of semver by providing an additional optional parameter that defaulted to the current behavior. I think this and issue #20 are great items for a v.2 and help make dotenv a more progressive configuration approach.

@vlucas
Copy link
Owner

vlucas commented Feb 5, 2014

#14 will go into v2.0.0 as well. I have already started on it.

@pda
Copy link
Contributor Author

pda commented Feb 5, 2014

@stanlemon My point is that optional parameters which turn on old broken behavior are complexity that a small semver codebase doesn't need to carry around with it :)

@lox
Copy link

lox commented Feb 11, 2014

Any word on this? Would be great to get this compatible with how the other major dotenv tools work.

@vlucas
Copy link
Owner

vlucas commented Feb 11, 2014

I'd like to see an $overwrite parameter somewhere so this behavior can be optional. Then I will merge it.

@pda
Copy link
Contributor Author

pda commented Feb 11, 2014

@vlucas It seems crazy to have an option which enables a bug. The semver bump to v2.0.0 will stop the change taking anybody by surprise.

The current simplicity is great; no optional behavior anywhere, just $path and $file to locate the file. It'd be great to keep it that way.

If you're certain you want to be able to opt-in to overwriting, I'll add that option, but I think it's a bad idea.

Cheers,
Paul

@lox
Copy link

lox commented Feb 15, 2014

FWIW, I agree with @pda. Something like dotenv behaviour should be dead standard across all platforms. Less magic is better.

vlucas added a commit that referenced this pull request Feb 15, 2014
Don't overwrite existing environment variables.
@vlucas vlucas merged commit ef2648a into vlucas:master Feb 15, 2014
@vlucas
Copy link
Owner

vlucas commented Feb 15, 2014

Okay. Agreed. I don't think this will break anything either.

@pda
Copy link
Contributor Author

pda commented Feb 15, 2014

Excellent, thanks @vlucas.

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

Successfully merging this pull request may close these issues.

None yet

4 participants