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

Config::get not working on a Command Extension #23

Closed
madd86 opened this issue Jan 3, 2017 · 10 comments
Closed

Config::get not working on a Command Extension #23

madd86 opened this issue Jan 3, 2017 · 10 comments

Comments

@madd86
Copy link

madd86 commented Jan 3, 2017

This is my command extension file:

<?php

namespace App\Console\Commands;
use Illuminate\Support\Facades\Mail;
use Illuminate\Support\Facades\Config;
use App\Mail\Recurr;

use Illuminate\Console\Command;

class SendEmails extends Command
{
    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'email:send';

    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'send email';

    /**
     * Create a new command instance.
     *
     * @return void
     */
    public function __construct()
    {
        parent::__construct();
    }

    /**
     * Execute the console command.
     *
     * @return mixed
     */
    public function handle()
    {            
        $this->info('Email: ' . Config::get('settings.contact_email_unpaid'));
    }
}

The idea is to send an email to the email configured.

Outputs Email: (null) for some reason..

@OwenMelbz
Copy link

Try adding this at the top

use Backpack\Settings\app\Models\Setting as Setting;

Then this before your code

        $settings = Setting::all();
        // bind all settings to the Laravel config, so you can call them like
        // Config::get('settings.contact_email')
        foreach ($settings as $key => $setting) {
            Config::set('settings.'.$setting->key, $setting->value);
        }

@madd86
Copy link
Author

madd86 commented Jan 3, 2017

Perfect, I would never get to that, thank you @OwenMelbz

@OwenMelbz
Copy link

Ha I only knew this because I started the PR which broke it xD

#19


This is a tricky one, coz basically there is an issue that the settings package tries to connect with the database every time the application starts. Including when you run things like php artisan optimize which is triggered from composer install/update/require as well as if your running unit tests etc etc.

This causes issues when there is no database present, e.g if a test suite is running, automatic deployments etc.

So a recent PR prevented the settings module from running when run from the command line, which as it happens affects your command.

We might need to re-factor that PR again a little, to enable something like Settings::boot which will enable it on per class/method level.

@tabacitu you got any ideas for this?

@tabacitu
Copy link
Member

tabacitu commented Jan 5, 2017

Damn. I don't have any bright ideas, I'm afraid.

  1. Maybe moving towards a syntax like Settings::get() like this PR Refactor to prevent eager db load #17 proposed. That would make it obvious you need to "use" that class, right?

  2. Using a flat-file storage for the Settings package? That would make it database-independent.

Can't say I like either of those very much, as they're breaking changes...

@OwenMelbz
Copy link

I think keeping the native syntax is probably nicest.

The benefit of using flat file storage is that it's much more easily source controlled and deployed, however would cause a tiny performance hit.

My personal approach would be to do something like

use Backpack\Settings;

public function __construct()
{
    Settings::init();
    parent::__construct();
}

Then we can enable users to use it on the CLI if they need by initialising it.

This I think is a similar approach to the step where you need to mock a class when running tests, so we just need to initialise it and it will work?

I cant see any other way really?

@pavoltanuska
Copy link

the settings package tries to connect with the database every time the application starts

@OwenMelbz is this really an issue when we check for the table existence?

I see there's already a check for that in place - count(Schema::getColumnListing('settings'), however, I'd prefer to use something like Schema::hasTable(with(new Setting)->getTable()) instead.

This would probably provide no benefit now, but would be independent from "settings" table name in the future, if there's need to change the name.

@OwenMelbz
Copy link

Hey,

Yeah it completely matters 😛

You cannot check for a table on a database that does not exist 😄

@pavoltanuska
Copy link

You're right. I should probably go home already, sorry :)

@OwenMelbz
Copy link

😂

@tabacitu
Copy link
Member

So, to sum it up, this is probably the best answer.

Closing the issue. Cheers!

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

No branches or pull requests

4 participants