cybercog / laravel-love

Add Social Reactions to Laravel Eloquent Models. It lets people express how they feel about the content. Fully customizable Weighted Reaction System & Reaction Type System with Like, Dislike and any other custom emotion types. Do you react?
https://komarev.com/sources/laravel-love
MIT License
1.16k stars 72 forks source link

custom database configuration is ignored #116

Closed vesper8 closed 5 years ago

vesper8 commented 5 years ago

Hi there!

I just upgraded to ^8.0 and I am noticing that the new ability to specify the database connection that was introduce in v7 is not working.

I have published the love.php config file and have changed the database like so:

    'storage' => [
        'database' => [
            'connection' => 'data',
        ],
    ],

data being a connection name for a database other than the default mysql connection

I have even added \Log::debug(Config::get('love.storage.database.connection')); inside your custom Migration.php file and it does correctly return data

However when performing the migrations, this setting is ignored and instead all tables are created under the default mysql connection

In order to work around the problem, I have set 'load_default_migrations' to false, I have copied your migrations outside of the vendor folder and into my application folder, and I have replaced all instances of Schema::create and Schema::drop by Schema::connection(config('love.storage.database.connection'))->create and Schema::connection(config('love.storage.database.connection'))->drop

And this works correctly, when performing the migrations it correctly creates the tables inside the data connection database

I have also replaced use Cog\Laravel\Love\Support\Database\Migration; by use Illuminate\Database\Migrations\Migration; since the custom one is no longer needed in this case

vesper8 commented 5 years ago

I am not sure why this is happening, I know you modelled this customization after Telescope's implementation on my recommendation.. it seems like Telescope's own implementation has since been modified like shown here: https://github.com/laravel/telescope/blob/ad74ce0499f5facb73f85ea0a0263b0d8cc0838c/src/Storage/migrations/2018_08_08_100000_create_telescope_entries_table.php#L9-L26

Using that, or applying the fix that I've used should remedy this problem

vesper8 commented 5 years ago

Furthermore... following my fix above, I was able to correctly run the command php artisan love:reaction-type-add --default which created two rows inside love_reaction_types table (inside my custom data connection)

However, following on with your setup guide, when I tried to run php artisan love:setup-reacterable --model="App\User" --nullable, I received the error Referenced table 'love_reacters' does not exists in database.

This is probably because my Users table and my Love tables are in two different databases. Perhaps this is intended and I will have to create that migration manually which is what I will try now.

antonkomarev commented 5 years ago

Hi @vesper8! Thank you for detailed information. What Laravel version do you have at this moment? It looks like getConnection() method is not being called on migration run. Will try to check it soon.

Accordingly setup reacterable\reactable models it's my fault. I'm checking for these tables without custom connection, this should be fixed:

if (!Schema::hasTable($referencedTable)) {
    $this->error(sprintf(
        'Referenced table `%s` does not exists in database.',
        $referencedTable
    ));

    return 1;
}
vesper8 commented 5 years ago

I'm on v5.8.35 at the moment with no rush to upgrade to v6

Happy to help, and thanks in advance for the fix! : )

antonkomarev commented 5 years ago

@vesper8 Laravel Love v8.1.1 has been released. I've created separate database for Love package and tested it... working well. There is only one issue - Laravel migration syntax doesn't support cross-schema foreign keys. You should delete foreign keys definition in migrations created from the Artisan setup commands.

antonkomarev commented 5 years ago

I've created PR to Laravel Framework to make Migrations respect getConnection() method: https://github.com/laravel/framework/pull/29983

jtomek commented 5 years ago

Taylor closed it. "No plans to change this right now."

antonkomarev commented 5 years ago

@jtomek adding this to framework was optional because we've got this issue solved by PR #118