anlutro / laravel-settings

Persistent settings in Laravel
MIT License
940 stars 112 forks source link

Dynamic database setting #153

Closed sausin closed 3 years ago

sausin commented 3 years ago

First off, thanks for the awesome package!! It's super straightforward to move from config to setting. I loved the feature to have defaults to start with!!

I'm trying to get it to work in a multi-tenant setup and it works if I'm using the setPath method to dynamically set up settings when using json store. However, if I try to go with database store, then there seem to be some issues and I get errors as below:

Caused by
PDOException: SQLSTATE[HY000]: General error: 1 no such table: settings

./vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:79
./vendor/laravel/framework/src/Illuminate/Database/Connection.php:352
./vendor/laravel/framework/src/Illuminate/Database/Connection.php:685
./vendor/laravel/framework/src/Illuminate/Database/Connection.php:652
./vendor/laravel/framework/src/Illuminate/Database/Connection.php:360
./vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:2351
./vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:2339
./vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:2905
./vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:2340
./vendor/anlutro/l4-settings/src/DatabaseSettingStore.php:228
./vendor/anlutro/l4-settings/src/SettingStore.php:252
./vendor/anlutro/l4-settings/src/SettingStore.php:232
./vendor/anlutro/l4-settings/src/SettingStore.php:123
./vendor/anlutro/l4-settings/src/helpers.php:11
./app/Console/Commands/SomeCommand.php:47
./vendor/laravel/framework/src/Illuminate/Container/Container.php:917
./vendor/laravel/framework/src/Illuminate/Container/Container.php:754
./vendor/laravel/framework/src/Illuminate/Foundation/Application.php:841
./vendor/laravel/framework/src/Illuminate/Container/Container.php:692
./vendor/laravel/framework/src/Illuminate/Foundation/Application.php:826
./vendor/laravel/framework/src/Illuminate/Console/Application.php:260
./vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:232
./vendor/laravel/framework/src/Illuminate/Console/Application.php:151
./vendor/laravel/framework/src/Illuminate/Console/Application.php:74
./vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:330
./vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:263
./vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:261
./tests/Feature/Commands/SomeCommandTest.php:102

The error line from the test is when I'm trying to use the setting helper to get a setting (in the constructor of the command). The settings.php config file as below relevant values:

    'store' => 'database',

    // If set to null, the default connection will be used.
    'connection' => null,

    // Name of the table used.
    'table' => 'settings',

When a new tenant is set, the following lines of code are run:

    Config::set('settings.connection', $this->tenantDatabaseConnectionName());
    Config::set('settings.store', 'database');

    App::singleton('anlutro\LaravelSettings\SettingsManager', function ($app) {
         return new SettingsManager($app);
    });

I understand that this is not completely related to the package as such, but any pointers in helping solve this issue would be awesome!

Thanks!

anlutro commented 3 years ago

PDOException: SQLSTATE[HY000]: General error: 1 no such table: settings

this error message is pretty clear: the table is missing from your database. based on your config snippet it seems like you might be running an individual database/schema for each of your tenants? if that's the case, and you want to keep settings in these tenant databases, you'll somehow need to make sure that the settings package's migrations are ran against all of those databases.

if you'd like to keep settings in a main database instead, you can pretty much follow this example from the readme, but replace user_id with tenant_id: https://github.com/anlutro/laravel-settings#example

sausin commented 3 years ago

@anlutro thanks for your reply! All the migrations are indeed already run for all tenants. What happens is that the database is switched depending on the tenant selected and the default database field corresponding to the tenant connection (default) is null. When the Setting singleton is bound by the application during bootup, there is no database selected and hence the error (as I understand it).

I was trying to re-bind the Setting singleton as shown in above code snippet after the database has been set in the background corresponding to the current tenant.

Your solution to use tenant_id instead of user_id will work as well - thank you for the suggestion! It's just that it would've been ideal to keep all tenant data in its own database.

anlutro commented 3 years ago

Aah, I see.. That doesn't explain why the error says there's no table settings though, but I'll assume there's an explanation for that.

I can't think of another immediate solution to your problem than the tenant_id in a "main" database. If you want/need to keep the settings in individual databases, I think you'll need to extend the DatabaseSettingStore class to keep an associative array of database connection objects rather than a single one, and then setConnection to control which connection should be used.

A simpler solution may be to add a setConnection method on the existing class, but that feels a bit iffy.

sausin commented 3 years ago

@anlutro setConnection could be awesome, but I agree that this is rather niche and may not warrant the effort on this package.

I'll try out the tenant_id solution for now. An alternate option (which I'd implemented for now) was to use the json store and have individual settings.json file for each tenant. This kept settings separately but introduced a future issue of folder sharing when the app will run on multiple servers.

Will close the issue as the original issue of storing in database is solved. On a per tenant setup, if something becomes possible I'll ping you here.

Thanks a lot!