Laravel 5.8 update breaks third-party composer libraries

[nomad-software]

• Laravel Version: 5.8.5
• PHP Version: 7.2.12
• Database Driver & Version: n/a

Description:

When upgrading our applications to use Laravel v5.8 there is a major breaking change in the way environment variables are being handled which renders useless the majority of third-party composer libraries that use PHP's getenv function.

This change is documented here: https://laravel.com/docs/5.8/upgrade which is great, and which we are using to upgrade our systems. Unfortunately this guide does not cover the actual change that was made in the underlying way the Dotenv library is being used. For whatever reason the adapter that was being used to populate the environment that getenv uses is no longer being loaded. This is a huge breaking change that effects an entire ecosystem of third-party libraries when being used with Laravel.

There was an issue raised here which was recently closed: https://github.com/laravel/framework/issues/27913 In that issue, the response was to use the Laravel env helper instead of PHP's getenv function. This is good advice for Laravel applications where you are in control of all code but for third-party composer libraries this is not possible. There are literally thousands of libraries out there that have no idea they are being used with Laravel and use getenv internally.

What this ultimately means is that it's impossible for us to upgrade any of our applications to Laravel 5.8 while this is broken.

Steps To Reproduce:

Use any code (including any third-party composer library) that uses getenv to load an environment variable from the .env file. It always returns false.

[driesvints]

getenv & putenv work just fine for any environment variable outside Laravel's .env environment. I've just tested this on a fresh 5.8 app. If you have an integration which relies on integrating with Laravel you should use a custom config file and use the env helper.

[driesvints]

Use any code (including any third-party composer library) that uses getenv to load an environment variable from the .env file. It always returns false.

This indeed won't work anymore but that's documented in the upgrade guide. In this situation you should replace getenv with the env helper.

[nomad-software]

getenv & putenv work just fine for any environment variable outside Laravel's .env environment.

Yes, I understand that but we are using Laravel here.

I've just tested this on a fresh 5.8 app. If you have an integration which relies on integrating with Laravel you should use a custom config file and use the env helper.

This would make sense if we are the ones who were responsible for the code. This code is in a third-party library that uses getenv throughout. We have no access to the code other than changing the vendor'ed dependency loaded through composer.

This indeed won't work anymore but that's documented in the upgrade guide. In this situation you should replace getenv with the env helper.

How can I do this if it's a third-party library? Also you are implying the library uses Laravel and has access to the helper. We are using an AWS library which is framework agnostic and will never be able to use the Laravel env helper. This also applies to hundreds, if not, thousands of framework agnostic PHP libraries that use getenv.

Do you understand that the Laravel env helper function is not available outside of the Laravel framework? Developers rely on the getenv function instead when writing a composer library that requires ENV variables.

Why has this been closed? This is a huge breaking change that affects lots of PHP libraries which essentially can't be used with Laravel now.

[driesvints]

You should ask the maintainer to update their library to support 5.8. Which library are we talking about exactly?

Can you provide a practical code example which demonstrate how this breaks your app? Please provide third party libraries in the exact version you're using them.

[nomad-software]

Amazon AWS SDK v3.90.4

I can't understand why there has been no deprecation notices regarding this or notices about such a huge change in functionality on a minor revision. This is disappointing coming from you guys.

[nomad-software]

Another related issue: https://github.com/laravel/framework/issues/27828

[driesvints]

Please link to the library in question.

[driesvints]

I can't understand why there has been no deprecation notices regarding this or notices about such a huge change in functionality on a minor revision. This is disappointing coming from you guys.

Please read our upgrade guide before upgrading.

[nomad-software]

Please link to the library in question.

Are you being serious? https://github.com/aws/aws-sdk-php

Here's another we're having the same problem with: https://github.com/braintree/braintree_php

Please read our upgrade guide before upgrading.

We were following it to upgrade our services. Nowhere does it say that the Laravel .env file is no longer populating the getenv PHP function and that any third-party libraries that rely on it will fail.

Maybe I can't see it, so can you point that part out please?

[nomad-software]

Here's a few more libraries we've identified as affected:

https://github.com/FriendsOfPHP/PHP-CS-Fixer https://github.com/googleapis/google-api-php-client-services https://github.com/vyuldashev/laravel-queue-rabbitmq

[driesvints]

Are you being serious? https://github.com/aws/aws-sdk-php

Yes, I was. I wanted to make sure we were talking about the same one. This library doesn't integrate with Laravel. There's a separate one which does but it's updated to be used with 5.8 and makes use of the env helper in its config file: https://github.com/aws/aws-sdk-php-laravel

Here's another we're having the same problem with: https://github.com/braintree/braintree_php

This library doesn't directly integrates with Laravel either.

Maybe I can't see it, so can you point that part out please?

The docs here: https://laravel.com/docs/5.8/upgrade#environment-variable-parsing link to https://github.com/vlucas/phpdotenv/blob/master/UPGRADING.md where at the bottom it details how we implemented the library according to their upgrade guide in a way not to call functions that aren't tread-safe (like getenv). But I can agree with you that this isn't super obvious. I'll send in a PR to the docs to make this more explicit.

[driesvints]

@nomad-software I really think you're missing the point of what's being discussed here. If you are using the .env file you should always use the env helper and not the getenv function. If you set env variables in any other way (CLI, php config) you can use the getenv function.

[nomad-software]

So we are only allowed to use libraries officially supported by Laravel now? You do realise that packagist contains are little more than that right?

I really think you're missing the point of what's being discussed here.

No you are missing the point. You are breaking developer's environments for no good reason (on a minor release I may add) and have not informed anyone of such a big change.

Why would you introduce a nice elegant way of handling environment variables (the .env file) that developers have been using for years then all of a sudden cripple its use and now advocate defining env vars in multiple different places? This is a huge step backward for no advantage. There is litterally no upside here. You've even broken your own server: https://github.com/laravel/framework/issues/27828

[AndyMac2508]

I have to agree with @nomad-software here , you are essentially saying that from now on all packages will need to have a version made to work with Laravel. I am also not seeing understanding why you are fighting a "FIX" that is literally adding two words to a method.

[driesvints]

I've tried being patient and helping to figuring out your problem but because of the negativity going on here and the remarks I've decided that this is where this conversation ends.

If you genuinely feel that there's a problem you are free to open up a new issue where we can discuss this in a calm and peaceful manner and I'll be happy to help you figure everything out.

[taylorotwell]

What is the fix? I'm not familiar with DotEnv 3.0.

[driesvints]

I've unlocked this again in a hopeful attempt we can perhaps resolve this peacefully.

[browner12]

As a reminder to all unaware users, Laravel does not follow semver, so 5.7 --> 5.8 is a major update, not a minor update.

https://laravel.com/docs/5.8/releases#versioning-scheme

[AndyMac2508]

We fixed it by altering the createDotenv function in the LoadEnvironmentVariables class to look like the below.

protected function createDotenv($app)
    {
        return Dotenv::create(
            $app->environmentPath(),
            $app->environmentFile(),
            new DotenvFactory([new EnvConstAdapter, new ServerConstAdapter, new PutenvAdapter])
        );
    }

Essentially just adding the PutenvAdapter object back into the DotenvFactory constructor this then tells DotEnv to push the env variables into the php enviroment by using putenv().

[CoolGoose]

I think Laravel developers know in general that Laravel does not follow semver. However this change would make the life of developers 'harder' by default, and make an upgrade path more complicated than it should be.

[driesvints]

@AndyMac2508 like said above this would re-introduce the issue about tread-safety as detailed in the phpdotenv upgrade guide: https://github.com/vlucas/phpdotenv/blob/master/UPGRADING.md

But at this point we're thinking about maybe adding the PutenvAdapter just for the sake of it since this seems to be affecting much more than the original PR considered.

For reference, here's the original PR and a detailed explanation of why these changes were made: https://github.com/laravel/framework/pull/27462

[browner12]

Don't disagree, but OP kept referring to this as a minor update, so I wanted to make sure everyone was on the same page.

[taylorotwell]

Gotcha, I don't mind adding the PutenvAdapter back in. Sorry, I'm on vacation with the family and just woke up to this thread. 😅

[taylorotwell]

I've released 5.8.6 with the PutEnvAdapter in place by default to be more similar to 5.7 behavior.

[barryvdh]

You should never rely on getenv being set by Laravel .env files, because if you cache the config file, the .env isn't loaded in production anymore.

[nomad-software]

@taylorotwell Thankyou.

You should never rely on getenv being set by Laravel .env files, because if you cache the config file, the .env isn't loaded in production anymore.

Yes but for dev, staging and test environments it's a god send.

[that-guy-iain]

As a "for the future", I think @driesvints should get more support when it comes to tickets like this. It's clear that he didn't understand the issue and closed the issue out of frustration that people were continuing to disagree with him.

[guice]

You should never rely on getenv being set by Laravel .env files, because if you cache the config file, the .env isn't loaded in production anymore.

Isn't the entire point of .env is to overwrite system env variables for development access?

We shouldn't rely on a .env within production, but .env is extremely useful for local development overrides. Let's assume it's a Docker container: system env will be set within docker, and .env is meant to override those values on a server-by-server instance.

In that instance ^^ Laravel will have to set/override the system env vars with all values supplied within .env.

[kieranbrown]

I asked for this a while back and Graham gave some valid reasons on why it was changed in the first place. See #27814.

Glad to see this was finally merged in though, it caused some issues for the setup I have now.

[kieranbrown]

@garygreen that's all well and fine in Laravel but illuminate/support is a generic package that can be used outside of Laravel. Let's take Lumen as an example, you can't config cache there.

[garygreen]

@Kieran-Brown That's true. I'm being simple minded, didn't realise third parties rely heavily on envs being defined in getenv etc, which I guess makes sense as it's framework agnostic.

[nomad-software]

@AndyMac2508 like said above this would re-introduce the issue about tread-safety as detailed in the phpdotenv upgrade guide: https://github.com/vlucas/phpdotenv/blob/master/UPGRADING.md

Which is more important: not breaking support for thousands of third-party libraries or being able to parallelize reading an .env file across multiple processors? :man_facepalming:

[GrahamCampbell]

This tweet explains why the change was made perfectly:

Third party libraries are using the wrong function. They should instead use the superglobals.

---

Which is more important: not breaking support for thousands of third-party libraries or being able to parallelize reading an .env file across multiple processors? 🤦‍♂️

Third party libraries are just calling the wrong thing. They should be updated to use the correct method of getting env variables safely. This is unrelated to Laravel.

[driesvints]

I've been going over the comments here a few times since the issue was opened again and want to say a few things. First of all, I'd like to apologies to you, @nomad-software for being too quick to judge and close and lock this issue. I now see how this would affect other libraries much better and it took a while before that sank in. I'm sorry for the way I handled this and I hope to be better at judging these situations in the future. Thank you for reporting this.

I do however, still want to voice my concern about the adding of the PutenvAdapter. I do agree with @GrahamCampbell and others that this isn't the most ideal situation and that the reasons stated in this thread as well as in the newly opened PR (https://github.com/laravel/framework/pull/27961) and the original issue here are valid and would be addressed by this.

However, I also acknowledge that this simply affects too much libraries atm. So maybe it's better if we provide an upgrade path for this and do this in a next version while letting people know up front a long time in advance.

[mnabialek]

Or maybe we could somehow make it configurable for now? to allow both new and old behaviour depending on preference? As I understand both ways are useful at the moment and for sure we don't have the power to force thousands of PHP libraries to switch to "correct" way

[collegeman]

I love Laravel. I love being a user of Laravel. The people who make Laravel are incredible, generous human beings. A buddy of mine sent this thread to me because he knows I have a lot of projects that depend on Laravel—I don't think he's doing any Laravel work himself, so it was super cool and generous of him to alert me to it. It shocks me zero that this issue was found, reported, discussed (passionately) and ultimately fixed. Huge props to everyone—restored a bit of faith in humanity today.

[nomad-software]

@driesvints

I've been going over the comments here a few times since the issue was opened again and want to say a few things. First of all, I'd like to apologies to you, @nomad-software for being too quick to judge and close and lock this issue.

Fair enough, apology accepted. Please, please, please try and understand the problem before closing tickets, many big open source projects do this and it drives me absolutely nuts.

I do however, still want to voice my concern about the adding of the PutenvAdapter. I do agree with @GrahamCampbell and others that this isn't the most ideal situation and that the reasons stated in this thread as well as in the newly opened PR (#27961) and the original issue here are valid and would be addressed by this.

If this is a major problem we need to take this up with the PHP devs instead of not using it. If that avenue of attack has been exhausted then, by all means, revisit this decision. If the decision is made again to remove it, please make it configurable. Then make sure everyone is aware it's going to happen and what the ramifications are. Write a blog post, spam reddit, post on the official Laravel twitter, etc. but please let people know.

Thanks anyway. We have got it sorted. :+1:

[nomad-software]

@GrahamCampbell

Third party libraries are using the wrong function. They should instead use the superglobals.

If it's in the standard library, it's going to get used. Frameworks need to deal with it.

Third party libraries are using the wrong function. They should instead use the superglobals.

Maybe you're the guy to correct the millions of PHP developers then?

[OwenMelbz]

Is the answer as simple as a config item to let people enable/disable threadsafe env vars?

[guice]

Third party libraries are using the wrong function. They should instead use the superglobals.

My two cents: I checked php.net - no mention about 1) not being thread-safe and 2) recommendations of using superglobals over PHP's own functions.

As long time PHP developer, this is honestly the first time I've ever heard of using a superglobal over php's built-in functions.

[ltsochev-dev]

Talking about thread safety on a non-threadding language. This must be a php-first haha

[devcircus]

On the contrary, he does an incredible job here and should be commended for his work across the Laravel ecosystem. I'm thankful that he continues the thankless job of open source pr/issue maintenance.

[OwenMelbz]

I have to vaguely agree with @deniamnet - however it’s not a problem isolated to him and shouldn’t be called out.

Historically everybody involved in Laravel is typically very abrupt and quick to shut people down, if you look at the typical attitude towards issues on github - there is generally a negative attitude, I’ve seen countless tickets by the whole team discarding people’s problems who do not subscribe to their beliefs. I’m not saying their reasons are not valid - however often unless they see it as a problem, they just won’t care - despite their business model being around providing a service to the public, it is why there is a sense of “Laravel elite” amongst the community where close friends share support between each other, and anybody else gets shut down immediately.

There’s countless tickets you can go through and see examples of this, the last 2 I can think of being on the Nova issues with the lack of breadcrumbs and being dismissed saying “use the back button” without having the knowledge or taking the time to understand the problems with back buttons, in fact the British government has whole research saying why you should provide your own. Another example is relationships in nova when attaching the same model twice causes a JavaScript error, but the issue was just closed because they couldn’t be bothered to explore the problem.

What will be a testament to the above, will be the fact the above will NOT get addressed, and it will be closed down almost immediately proving the point 😂

[mfn]

Here is another data point: due to the popularity, the number of "low effort" issues and PR is insane at times:

• People not able to read the basic template and create a issue without any information
• People creating bug report issues which are actually support requests best asked somewhere else (even though there are clear links / hints)
• People creating PRs with breaking change left and right and no tests until nudged, etc.

Ah well, and sometimes you simply misjudge an issue. Human error. It's natural. Happens all the time. To everyone.

Somewhere, forgot where, Dries was called out. I was shocked about the unfair treatment. IMHO he does a great at polite job, all the times. But still, like everyone else, he's just a human. Luckily 😄

[danjdewhurst]

IMHO he does a great at polite job, all the times - mfn

Apparently not, from reading the above comments and related issues. The issue was closed without proper thought and @nomad-software was constantly told he was wrong. I know it must be frustrating that lots of people (including myself) sometimes open issues that are incorrectly formatted or a number of other reasons, which ultimately waste time, but that's no excuse to shut someone down without properly hearing them out.

This isn't a poke at anyone, I'm just trying to say that people need to be understanding and to not be too quick at jumping to conclusions and shutting conversations down.

I'm glad this is getting resolved though, thanks to all the contributors!

[khalwat]

Dotenvy is a package I wrote that generates server environment variables from your .env files: https://github.com/nystudio107/dotenvy

[hopeseekr]

I have liberated Laravel 5.7's env() from the dustbin of the git repo and made it its own package. What's more, using some real composer hacks, I have ensured that it will always be autoloaded first, overriding whatever Laravel's current env() does.

Benefits:

• Works with any package, even non-Laravel ones (it was my secondary reason)
• You no longer have to fear Laravel just going one day "Oops! We're not going to read from the environment anymore! Too bad!" on a non-major release.

https://github.com/phpexpertsinc/Laravel57-env-polyfill

To install: composer require phpexperts/laravel-env-polyfill.

[browner12]

I'll say it again for the people in the back....

5.7 --> 5.8 IS A MAJOR RELEASE