-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Replace getenv() with env() #1121
Conversation
So If it doesn't change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is a namespaced equivalent : https://github.com/illuminate/support/blob/master/Env.php
I think it would be better long term...
I would say we should just follow the same pattern as Laravel, which uses the They do use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough
Codecov Report
@@ Coverage Diff @@
## hotfix #1121 +/- ##
=========================================
Coverage 67.81% 67.81%
+ Complexity 1981 1978 -3
=========================================
Files 170 170
Lines 6926 6926
=========================================
Hits 4697 4697
Misses 2229 2229
Continue to review full report at Codecov.
|
Note there was actually an issue with this PR that appeared after merge. See temp fix in 926b9be Error : https://github.com/userfrosting/UserFrosting/actions/runs/628080953 Looks like env doesn't use the loaded env right above : UserFrosting/app/sprinkles/core/src/ServicesProvider/ServicesProvider.php Lines 235 to 245 in 926b9be
|
See #1098. Not sure if we want to merge this in for a revision or save for another minor version.