framework: Laravel 5.8 update breaks third-party composer libraries

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.

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.

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

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.

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.

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

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

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.

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.

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.

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.

@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.

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.

Please link to the library in question.

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.

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.

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().

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

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 😄

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.

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.

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?

@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. 👍

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

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.

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.

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

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. 😅

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

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

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.

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!

@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.

@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.

@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

I’ll say it again for the people in the back…

5.7 --> 5.8 IS A MAJOR RELEASE

@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?

@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? 🤦‍♂️

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.

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.

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

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

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.