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

Populate dotenv to config #409

Closed
wants to merge 7 commits into from
Closed

Populate dotenv to config #409

wants to merge 7 commits into from

Conversation

BernardGoldberger
Copy link
Contributor

I noticed that occasionally I would have an error

[2016-08-10 00:23:08] production.ERROR: PDOException: SQLSTATE[HY000] [1045] Access denied for user 'forge'@'localhost' (using password: NO) in C:\wamp64\www\koel\vendor\laravel\framework\src\Illuminate\Database\Connectors\Connector.php:55

of course this make no sense since I have set the DB and password.

So I did some research and found 2 things

  1. dotenv should only be used in development, once in production it should all be cached.
  2. dotenv should only be populated in the config directory.

Here is a post vlucas/phpdotenv#76 (comment) by @GrahamCampbell about this.

In this PR I created a separate config file koel.php for all Koel specific configurations, and I also fix #408.

Going forward we can tell users to run php artisan config:cache once they finnished setting up Koel, or we could even make part of the koel:init command.

Note

I think this might in fact be the actual issue with #393.

if (!env('ADMIN_NAME') || !env('ADMIN_EMAIL') || !env('ADMIN_PASSWORD')) {
if (!config('koel.admin.name') ||
!config('koel.admin.email') ||
!config('koel.admin.password'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected 1 space after closing parenthesis; found 9

if (app()->environment() !== 'testing') {
$this->command->info('Admin user created. You can (and should) remove the auth details from .env now.');
if (! app()->runningUnitTests()) {
$this->command->info('Admin user created. You can (and should) remove the auth details from .env now.');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line indented incorrectly; expected at least 12 spaces, found 8

@BernardGoldberger
Copy link
Contributor Author

I will fix it all up.

'admin' => [
'name' => env('ADMIN_NAME'),
'email' => env('ADMIN_EMAIL'),
'password' => env('ADMIN_PASSWORD'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style.

@phanan
Copy link
Member

phanan commented Aug 17, 2016

This looks great! And thank you for your findings – I learned something new today. Can you

  • Resolve the conflicts
  • Squash the commits into one
  • Separate the seeder into another PR?

@BernardGoldberger
Copy link
Contributor Author

I'm not that good at this :-(

Close this PR and I will make them from scratch.

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.

Bug: artisan config:cache
3 participants