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

Use Env::get instead of env #745

Merged
merged 1 commit into from
Sep 12, 2020
Merged

Use Env::get instead of env #745

merged 1 commit into from
Sep 12, 2020

Conversation

oytuntez
Copy link
Contributor

It conflicts with other env() global functions (such as CakePHP), and most of them don't handle closures.

It conflicts with other `env()` global functions (such as CakePHP), and most of them don't handle closures.
@oytuntez
Copy link
Contributor Author

This was hell of a debug run. I resolved it by requiring Laravel helpers (why global!) before composer autoload.

@mcamara mcamara merged commit fdb68d2 into mcamara:master Sep 12, 2020
@mcamara
Copy link
Owner

mcamara commented Sep 12, 2020

@oytuntez thanks for your pr, I’ll create a new release soon.

@JaZo
Copy link

JaZo commented Sep 14, 2020

Please note this broke Laravel <6 support as the Illuminate\Support\Env is only introduced in Laravel 6.

@mcamara
Copy link
Owner

mcamara commented Sep 14, 2020

thanks @JaZo

@oytuntez can you create a laravel version checker for the if? I would like to maintain both versions of it. If not, I'll revert the PR

@oytuntez
Copy link
Contributor Author

@JaZo sorry about that!

@mcamara I created another PR for version check. It duplicated a bunch of lines... It was a quick edit on GitHub UI 🤕

@iwasherefirst2
Copy link
Collaborator

iwasherefirst2 commented Sep 20, 2020

@oytuntez I actually don't understand why the global env function from Laravel has to be replaced in this package. It may conflict with CakePHP but thats a different framework. This plugin is for Laravel framework, why should you make it compatible with CakePHP? The env helper is an essential part of Laravel: https://laravel.com/docs/configuration

@oytuntez
Copy link
Contributor Author

@iwasherefirst2 right, I actually resolved this issue by requiring Laravel's helper functions file earlier than CakePHP's (used in phinx). I don't really need this change at this point.

There was a back and forth with CakePHP and Laravel repos about this global namespace issue; anyone using phinx in Laravel ecosystem is affected by this.

@oytuntez
Copy link
Contributor Author

but by the time I started loading helpers earlier, I had already opened the PR and merged, so I had to follow up with another PR to fix my previous compatibility error.

@tontonsb
Copy link

@oytuntez I actually don't understand why the global env function from Laravel has to be replaced in this package. It may conflict with CakePHP but thats a different framework. This plugin is for Laravel framework, why should you make it compatible with CakePHP? The env helper is an essential part of Laravel: https://laravel.com/docs/configuration

Even though I came here because my Laravel 5.8 project broke because of this, I support replacement of env. That function should not be used according to Laravel docs as it stops working when you cache the configuration:

If you execute the config:cache command during your deployment process, you should be sure that you are only calling the env function from within your configuration files. Once the configuration has been cached, the .env file will not be loaded and all calls to the env function will return null.

@iwasherefirst2
Copy link
Collaborator

@tontonsb It is a Laravel function and it is certainly recommended to use it :) The part that you quotet just mentioned that you shouldshould only call env in your configurations files, and in your application you should call config(...).

@oytuntez
Copy link
Contributor Author

@iwasherefirst2 that's what @tontonsb meant, I think. It is used in a risky way as it is.

I have another PR pending, let's merge that and resolve the pain for L<6 users.

@iwasherefirst2
Copy link
Collaborator

@oytuntez its not risky. Its recommended to use it in the official docs. Its also recommended to not use it outside of config files to allow caching and improve application performance. Nothing wrong with env() wrapper and its used for Laravel 4,5,6,7,8 so we are all good :)

@nxu
Copy link

nxu commented Sep 30, 2020

Can confirm, it's broken on Laravel 5.7.

Also, as previously noted, please consider not using env() anywhere outside of config files. You typically want to cache configuration values in production, and env() always returns null when the config is cached as the .env file will not be loaded at all:

https://laravel.com/docs/8.x/configuration#configuration-caching

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.

6 participants