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

[7.x] Don't enable immutability when loading the .env file #30926

Closed
wants to merge 1 commit into from

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Dec 24, 2019

Since Laravel has had .env files, immutability has always been enabled, although it was not always called that, it's always been there. I propose in Laravel 7, we stop using this.

The naming is a little confusing... The difference between mutability and immutability is that if an environment variable was already set before the .envfile was loaded, then the variable from the file will be ignored if we are in "immutable" mode.

TLDR: mutable vs immutable is actually about if the .env file loading can overwrite existing variables. Most people will probably want mutable mode, and wouldn't even be aware that immutable mode was a thing.


Twitter poll: https://twitter.com/GrahamJCampbell/status/1210543678835634179.

@GrahamCampbell
Copy link
Member Author

Some initial feedback:

image

@Miguel-Serejo
Copy link

Does this need to be changed? Do people commonly run into problems with this feature?
How does this affect people who don't rely solely on .env for their environment variables?

One use case I've seen is being able to set DEBUG=true for a single artisan command call to catch a bug in production. This of course only works if you're not caching your config, but I'd bet a good amount of people who don't know about .env immutability also don't know about config caching, while people who rely on immutability probably do know about it, but consciously choose not to do it to allow for this kind of versatility.

@GrahamCampbell
Copy link
Member Author

@36864 After this change, what you want can actually already be done with the --env commandline flag.

@taylorotwell
Copy link
Member

I'm also curious what we are actually fixing here? What situations are people complaining about now that this will allow?

@GrahamCampbell
Copy link
Member Author

GrahamCampbell commented Dec 27, 2019

It's about choosing which behaviour we want. Do we want:

  1. Read the dotenv file and set all the variables we read.
  2. Read the dotenv file and set only the variables that don't already have values due to some other mechanism.

Currently, we have the 2nd behaviour. I'm proposing we switch to the first behaviour.

@Miguel-Serejo
Copy link

@36864 After this change, what you want can actually already be done with the --env commandline flag.

As long as there's an easy and documented workaround I don't have strong objections, but I still don't see the need to change this.

It's about choosing which behaviour we want.

I assume this was made immutable instead of mutable for specific reasons when it was first implemented. Are those reasons no longer valid? I traced it back to #27519 but I'm not sure what the behavior was before that PR.

@GrahamCampbell
Copy link
Member Author

I traced it back to #27519 but I'm not sure what the behavior was before that PR.

That's not immutable vs mutable (previously called load vs overload). That PR was all about simply where we look for env variables.

@GrahamCampbell
Copy link
Member Author

I assume this was made immutable instead of mutable for specific reasons when it was first implemented.

Historically, mutable was never an option, and laravel has used the "load" function of phpdotenv since forever. "overload" was then added to phpdotenv, and later was refactored to the terminology of the internals being mutable or immutable (I don't like the name now, but I don't wanna do another major version bump to change it right now).

The question now is if Laravel wants to start using mutable mode (previously called overload).

@sisve
Copy link
Contributor

sisve commented Dec 28, 2019

So, with this change, how would I override a single option? I often do QUEUE_DRIVER=sync php artisan my:command to have all jobs dispatched by the command executed by the sync queue, instead of whatever is the .env file. This depends on the fact that we do not currently overwrite existing environment variables.

@GrahamCampbell
Copy link
Member Author

Ok, so, somebody needs to provide me with a use case for why this should be merged, so I can compare. Otherwise, I think I'll close this.

@GrahamCampbell
Copy link
Member Author

Oh, yeh. I remember what the reason for merging was. If someone is running tinker, or any other server that keeps the process alive, if someone edits the env file, then the changes only will show up if this PR is merged. Otherwise, phpdotenv thinks the env variables are already loaded after the first time, so doesn't load them again.

@GrahamCampbell
Copy link
Member Author

I was going to close this, but actually, I'll leave this to Taylor to have the final say.

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.

4 participants