replacing getenv($key) with array_get($_ENV, $key)

[janhartigan]

We've been having some issues on some local Windows environments with the new dotenv library.

When you make rapid, successive AJAX requests to a Laravel app, some of the later requests in this chain seem to carry over putenv() values from a previous request, but only until the previous request is finished. This sounds strange, I know, but maybe a diagram can help:

Request 1: {starts --- env loaded --- app uses env ---- finishes}
Request 2:                         {starts --- env loaded ------ app uses env ---------finishes}

When finishes happens, PHP clears the values that were set using putenv(), but this appears to be affecting values across multiple requests from the same session.

Debugging Dotenv shows us that in the second request, it gets here and the environment variable is found...but only when you ask for it from getenv(). It's not available in $_ENV or $_SERVER. Then as the first request dies, it seems to clear out the values previously stored via putenv(). I'm not an expert on the PHP innards in Windows, but I suspect this is a bug related to sharing environment data between requests from the same session.

So the reason I'm creating this issue here is because Laravel's env() helper uses getenv(). Unfortunately in our environment, by the time the second request gets there, it's already been unset from the first request. We can tell Dotenv to be mutable, thus ensuring the values get stored in all three environment variable types, but that doesn't help us if Laravel still uses getenv(). We could create our own env() helper, but then we won't be able to keep up to date with any changes to Laravel's.

This PR represents what I think is a change that won't affect Laravel in any other environments, but will solve this problem in our Windows environment. Any alternate solution is welcome if this one isn't palatable for some reason.

[toddbc]

I've experienced a related problem as well. However, in my case $_ENV did not have the values, and getenv() always reliably did. The problem was completely resolved by removing any reference to $_ENV in the code, and always using the env() helper (the one that uses getenv(). The environment was Apache using mod_ssl on Windows.

Dotenv being mutable would impact unit tests, and also environment setups in load balanced clusters (where you might override .env with local environment variables.) So, that's not a solution indeed.

In my case, it seemed to be the opposite problem - $_ENV was only empty for the first of two successive http requests. Luckily, it was consistently reproducible. If there's some overlap problem, perhaps this was on the other end of it.

It's sounding like this is possibly a bug in php itself.

[GrahamCampbell]

We've been having some issues on some local Windows environments with the new dotenv library.

They should fix the issue then.

[GrahamCampbell]

This pull requests probably overall makes the situation worse.

[crynobone]

Why not

$value = array_get($_ENV, $key, getenv($key));

[frankiecmk]

@crynobone good solution 👍

[frankiecmk]

We've been having some issues on some local Windows environments with the new dotenv library.

I'm facing this issue on Linux AMI - AWS
PHP 5.5.22 (cli)
Server version: Apache/2.4.10 (Amazon)

[toddbc]

This shouldn't happen with cli, I think. Are you using it in Apache as cgi or a module? Which mpm are you using (prefork, worker, etc. - if you don't know, httpd -V | grep MPM should tell you, I think)?

I don't believe we've experienced it with php-fpm.

[frankiecmk]

@toddbc : apache using as module, Server MPM: event

[toddbc]

That's probably why. Env vars are traditionally per-process, so putenv/getenv are not really "thread safe" when you have separate collections. The winnt and event mpms are both multithreaded rather than multiprocess.

So basically the issue is that use of Dotenv, given that it modifies the environment using putenv(), is not really safe on multithreaded webservers. This is probably causing the weird behavior with $_ENV that I saw (maybe based on when the jit subglobal is built?)

That would imply that changing Dotenv to not use putenv() (which would mean it would not affect forked child processes, but they would reload Dotenv anyway) and changing Laravel not to use getenv() (using $_ENV instead) would be the ticket. This would prevent any actual modifications to the environment.

Also, I'm not sure what functions PHP uses to set the environment. They're not all threadsafe or re-entrant in Linux, for example, iirc.

[toddbc]

I've created an issue under Dotenv - vlucas/phpdotenv#76.

Depending on your position, this is a PHP bug (what are they supposed to do?), but I'd say as above that modifying the environment in the first place is just a bad idea. If Dotenv changes (not sure if it will), Laravel would also have to change (and this pull request would be correct, I suppose.)

[progmars]

I tried the @crynobone 's code but it did not work for me. While debugging, I found out that in my case when things break down for concurrent requests, the $key cannot be found nor in getenv(), nor in $_ENV and not even in $_SERVER.
The only thing that worked was to modify Dotenv class.
I added:

 protected static $cache = [];

then modified setEnvironmentVariable adding the following block right after list($name, $value) ...

    // workaround for 
    // https://github.com/laravel/framework/pull/8187
    // and
    // https://github.com/vlucas/phpdotenv/issues/76

    // don't rely upon findEnvironmentVariable because there might be situations
    // when getenv() or $_ENV is set, but $cached[$name] is not, 
    // and then for later requests getenv() and $_ENV are both empty and also no value in cache,
    // therefore this additional fix is here to ensure that we always cache the value

    // but first keep the value while we haven't updated the cache because after that the value will be returned from the cache
    // thus completely ignoring ENV, which is not what we intended
    $oldVal = static::findEnvironmentVariable($name);

    if (static::$immutable === false || !array_key_exists($name, static::$cache)){
        static::$cache[$name] = $value;
    }

and then in findEnvironmentVariable I added

case array_key_exists($name, static::$cache):
            return static::$cache[$name];

and call Dotenv::findEnvironmentVariable($key) everywhere where you would normally call getenv() (for example, in Laravel's helpers.php env() function, and replacing 'false' check with 'null' check).

Although Dotenv was not meant for production, I don't want to change our deployment and configuration workflow.

With my workaround I was able to run apache bench and also concurrent AJAX requests (queued with setInterval() ) for an hour and did not get any issues with Dotenv (before my fixes, I had about one crash each minute). So, now it seems Dotenv's findEnvironmentVariable() is more reliable than PHP's getenv().

[barryvdh]

I've created an issue about creating an read-only mode, without putenv upstream in Dotenv; vlucas/phpdotenv#163

[progmars]

Now with the latest Laravel and Dotenv my fix doesn't work anymore because Dotenv::findEnvironmentVariable no longer exists, so I can't use it in Laravel's env() function. But I found out that on my Windows machine the following workaround works reliably:

Laravel vendor/laravel/framework/src/Illuminate/Foundation/helpers.php file:

    function env($key, $default = null)
    {
        $value = getenv($key);

        // start of fix
        if($value === false){
            // try extracting from $_ENV or $_SERVER and give up finally
            $value = array_key_exists($key, $_ENV) ? $_ENV[$key] :
                (array_key_exists($key, $_SERVER) ? $_SERVER[$key] : false);
        }
        // end of fix

        if ($value === false) {
            return value($default);
        }

Although hacky, but still it seems logical, considering that Dotenv puts variables in all three locations:

        putenv("$name=$value");

        $_ENV[$name] = $value;
        $_SERVER[$name] = $value;

However, for production now I'm using the Laravel's recommended approach (since 5.2) with config:cache and no .env at all.

[petarthewizard]

@progmars so "php artisan config:cache" gets rid of the thread safety issues?

[JustinShift]

Additional to php artisan config:cache suggestions, I believe turning off clear_env = yes in your PHP-FPM pool config could be another option.