.env containing duplicates provides confusing outcome

[miketheman]

Hello! Thanks for your library - it's very helpful.

I recently came across a confusing situation, where I had inadvertently set the same variable twice in a .env file with different values (as part of a Laravel deployment).

Example:

APP_DEBUG=false
....
APP_DEBUG=true

Results in APP_DEBUG=false after loading.

I totally realize that it's my responsibility to ensure that I'm not setting these twice, however I was surprised that there wasn't something that would either warn me about a duplicate, or use the last-set value, since it was set later?

Is this something that should be handled in the library? I couldn't find an associated test that expresses this concept.

[GrahamCampbell]

Thanks for getting in touch. We have no mechanism of issuing a warning. We can either crash, or do nothing. It's interesting that the last value wasn't the one that ended up being used. I'll have a think about what is the best approach to address this.

[hopeseekr]

The last value should always be used.

There should NOT be a crash.

This is how exported bash variables work, which is what this project aims to mimic, and it's the behavior pattern most expected by end-devs.

[GrahamCampbell]

Right. I wonder why the last value is not being used in this case...

[GrahamCampbell]

Each name-value entry is processed in the order they were found:

https://github.com/vlucas/phpdotenv/blob/v3.3.3/src/Loader.php#L152-L175

[TheDigitalOrchard]

If you're processing one line at a time, and not in overload mode, then the first value encountered will take precedence, correct? Maybe you need to enforce overload mode while processing lines?

In other words, the loader needs to set as immutable during this?

[TheDigitalOrchard]

Since overloading is an option that you may not want to enforce, maybe the loader can keep track of the keys already loaded (during a single load()), and only go into overload mode when it encounters an existing key.

[GrahamCampbell]

Right, I see. Maybe the behaviour we have is correct, and if people want to have the other behaviour, they should run in overload mode?

[hopeseekr]

If this is to emulate shell Environment varialbes, so that one can do . .env; mysql -u $DB_USER --password=$DB_PASS and easily load into docker environment variables, then you should strive for bash compatibility, which means that the last overwrites former.

If this project's goal is NOT to emulate shell env scripts, then we will have a situation like how the Laravel devs decided, unilaterally, in v5.8 to stop reading the env variables from, you know, the environment. That's why I created phpexpertsinc/Laravel57-env-polyfill (plus you can now use env() with vlucas/phpdotenv without needing Laravel).

[Dzhuneyt]

As a developer, I expect the last value to override the first one OR a crash. Anything else is counter-intuitive.

[GrahamCampbell]

hat's why I created phpexpertsinc/Laravel57-env-polyfill

I don't understand why you created that? Laravel 5.8 behaves in exactly the same way as 5.7 for environment variable reading (after an initial wobble that was reverted).

[GrahamCampbell]

in v5.8 to stop reading the env variables from, you know, the environment

That change was revered, but you misrepresent the motive for the change in Laravel. It change was just to avoid calling non-threadsafe functions. It still read from both the server and env superglobals. Anyway, as I said, out of the box, Laravel 5.8 does now behave just as 5.7 did.

[GrahamCampbell]

Alright, this is fixed in v3.4.0, by 5084b23845c24dbff8ac6c204290c341e4776c92. Thanks for the report @miketheman. The issue was due to load not overriding existing variables by default, but it was mistaking also not overriding variables that were originally unset, but were set in the env file. This has been fixed now, so if a variable was initially unset, and was set by the env file, it can then be overwritten again later in the env file. The behaviour should be explained by the new test I've added. :)

[miketheman]

Thanks @GrahamCampbell and everyone else who put effort into this. Very happy with the resolution.

[GrahamCampbell]

🚀🚀🚀

[martinssipenko]

@GrahamCampbell isn't this a breaking change? Given order is treated differently that it was before, configurations that relied on that are not working anymore.

[GrahamCampbell]

I'd regard this as a bug fix. The intended behaviour was definitely for the last item to persist.

Even if we regard it as breaking, if we change it back, this will also break people's code, since there release has been out for a bit now. :/

[martinssipenko]

Fair enough, but I think there could be far more projects depending on pre 3.4 order than there are post 3.4. Even if intended behaviour was for last to persist it 3.4 has a major braking change and simple composer update just breaks apps in some cases. It took us 16 days to notice this, quite a pong time, but its not that every day we update dependancies. We rely on order in .env files because we load config values from various sources - app specific and global, and wanted app specific to be more powerful. Also, the order of persistance was not documented when we built upon this library, so by testing behaviour we assumed its first occurance wins.

Can I suggest reverting to pre 3.4 behaviour and making order configurable, and having post 3.4 behaviour for 4.x?

[GrahamCampbell]

Can I ask, why do you have more than one entry of the same key in a file?

[GrahamCampbell]

I'd like to understand the use case for this.

[martinssipenko]

We populate .env file with values from various sources.

[martinssipenko]

Let me elaborate, on deployment, we have a script that creates .env file by populating it with values from various sources. The sources are app specific values stack values and global values in that order. Pre 3.4 behaviour guaranteed that we can "override" global values in app context as app values are before global values in .env. file.

[GrahamCampbell]

Hmmm. I'm still unsure what to do. The order of loading was not documented or tested until v3.4, so one could argue the behaviour was never properly specified.. Reverting it in v3.5 could be very confusing.

[martinssipenko]

I do see your point, but having the change in behaviour also is very confusing.

[Dzhuneyt]

Given both are confusing, I would propose we move forward with the change that makes sense and is intuitive, document it, and forget about it. :)

A third party that depends on this behavior is free to use an older (fixed) version until they have time to migrate their side of the code to adapt to this behavior.

[martinssipenko]

Of course we can use older version, but essentially this can be considered a braking change and given semantic versioning is used (which is industry and community approved standard) having this behavioural change in 3.x is regression.

[GrahamCampbell]

It could also not be considered a breaking change, because the correct behaviour was never specified. This is why bug fixes are not considered breaking changes.

[martinssipenko]

Bug fix or not this drastically changes the behaviour in non backwards compatible way, all versions prior 3.5 (including 2.x) used reverse order. I'm not arguing that this should not be fixed. Furthermore, we will stick to 3.4 and update our scripts to fix order.

At the end of the day, there are 70 millions of installs of this library and simple composer update breaks the behaviour as it was observed before. Even if this has not been documented before I think it's false to assume that this is simply a "bug fix", given how many people this can affect.

[Dzhuneyt]

70 million downloads, true, but we are only talking about an extreme edge case here. Variables override is not a common use case for this library.