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

[5.9] Add Env class #28073

Merged
merged 1 commit into from
Apr 1, 2019
Merged

[5.9] Add Env class #28073

merged 1 commit into from
Apr 1, 2019

Conversation

kawax
Copy link
Contributor

@kawax kawax commented Mar 30, 2019

Again.
#28061

If other packages have global env(), the Laravel project will be broken. It actually happened.
The dependency package may have env() even if I do not install it myself.
This PR gives the option not to use env().

<?php

use Illuminate\Support\Env;

return [
    'name' => Env::get('APP_NAME', 'Laravel'),
];

It is necessary to consider whether to deprecate env().

Example of conflict

APP_DEBUG=false

Laravel env()
(bool)false

Another env()
(string)false
==(bool)true

Debug mode is enabled in production. Very dangerous.

@driesvints driesvints changed the title [5.9]Add Env class [5.9] Add Env class Mar 30, 2019
@driesvints
Copy link
Member

Please provide a thorough explanation in your original comment why this is needed and how it helps users and not just link to another issue/pr.

@kawax
Copy link
Contributor Author

kawax commented Mar 31, 2019

Currently, the frequency of encountering conflicts is low, so it is sufficient to be able to choose env() or Env.

@BrandonSurowiec
Copy link
Contributor

BrandonSurowiec commented Mar 31, 2019

Should there be a callback to customize the DotenvFactory parameters?

Env::setAdapters([
    new EnvConstAdapter,
    new PutenvAdapter,
    new ServerConstAdapter,
]);

Env::getAdapters(); // [new EnvConstAdapter, new PutenvAdapter, new ServerConstAdapter]

If a developer wants to customize the adapters in their AppServiceProvider it would allow them to do so.

The adapters are also referenced here:

https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Bootstrap/LoadEnvironmentVariables.php#L92

It would be nice to centralize the adapters, but I'm unsure if customizing the callback in the AppServiceProvider would be early enough for the LoadEnvironmentVariables@createDotenv call.

@garygreen
Copy link
Contributor

garygreen commented Mar 31, 2019

+1 from me. It's a nice way of cleaning up the code in helpers.php which often is a hive of random code and can be a cause of PR conflicts if lots of people touch it. I personally think helpers.php should just contain minimal code which hook into a class.

It also opens up possibilities of customising Env as @BrandonSurowiec mentioned, which could help potentially fix #27961 by giving devs an opporutnity to add PutenvAdapter if they need to support third party apps.

@taylorotwell taylorotwell merged commit d8b45c6 into laravel:master Apr 1, 2019
@driesvints driesvints mentioned this pull request Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants