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

Laravel 5.8 update breaks third-party composer libraries #27949

Closed
nomad-software opened this Issue Mar 21, 2019 · 46 comments

Comments

Projects
None yet
@nomad-software
Copy link

commented Mar 21, 2019

  • 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: #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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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 driesvints closed this Mar 21, 2019

@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

Another related issue: #27828

@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Please link to the library in question.

@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

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

This comment has been minimized.

@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

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: #27828

@AndyMac2508

This comment has been minimized.

Copy link

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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.

@laravel laravel locked as too heated and limited conversation to collaborators Mar 21, 2019

@taylorotwell taylorotwell reopened this Mar 21, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

@browner12

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

This comment has been minimized.

Copy link

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@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: #27462

@browner12

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

@barryvdh

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

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

@iaicam

This comment has been minimized.

Copy link

commented Mar 21, 2019

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

This comment has been minimized.

Copy link

commented Mar 21, 2019

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.

@Kieran-Brown

This comment has been minimized.

Copy link

commented Mar 21, 2019

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.

@Kieran-Brown

This comment has been minimized.

Copy link

commented Mar 21, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

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

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

This tweet explains why the change was made perfectly:

image

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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 (#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

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

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

@nomad-software

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

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

@guice

This comment has been minimized.

Copy link

commented Mar 21, 2019

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.

@Naminator

This comment has been minimized.

Copy link

commented Mar 22, 2019

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

@devcircus

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

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

This comment has been minimized.

Copy link

commented Mar 22, 2019

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

This comment has been minimized.

Copy link

commented Mar 22, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.