jdavidbakr / mail-tracker

Package for Laravel to inject tracking code into outgoing emails.
MIT License
577 stars 129 forks source link

the new mail-tracker:migrate-recipients command does not respect custom connections #133

Closed vesper8 closed 3 years ago

vesper8 commented 3 years ago

Simple fix

from

DB::table('sent_emails')->orderBy('id')->chunk(100, function ($emails) use ($bar) {

to

DB::connection((new SentEmail)->getConnectionName())->table('sent_emails')->orderBy('id')->chunk(100, function ($emails) use ($bar) {

Thanks!

jdavidbakr commented 3 years ago

Oops, thanks for the report. Fixed in 5.0.1

vesper8 commented 3 years ago

Thank you @jdavidbakr

I also noticed that you are not dropping the old sender/receiver columns as part of your new migration fixes. This is problematic for me because of how I handle migrations which is different than the normal flow.

I've written a script to manually drop them whenever I re-run all my migrations otherwise I lose all the data in the sent_emails table as it tries to insert data in the sender/receiver column which no longer exist as they are not present in your new migrations but they are present in my database backups.

It would have been better if you had a "dropColumnIfExists" logic for both of those columns. It would save me some headaches if you could add that too. Here's a snippet that should do the trick.

Thinking further though, this would have to be run manually once following the mail-tracker:migrate-recipients.. it could be part of the migration helper itself.

Many thanks!

if (Schema::connection((new SentEmail)->getConnectionName())->hasColumn('sent_emails', 'sender')) {
    Schema::connection((new SentEmail)->getConnectionName())->table('sent_emails', function (Blueprint $table) {
        $table->dropColumn('sender');
    });
}

if (Schema::connection((new SentEmail)->getConnectionName())->hasColumn('sent_emails', 'recipient')) {
    Schema::connection((new SentEmail)->getConnectionName())->table('sent_emails', function (Blueprint $table) {
        $table->dropColumn('recipient');
    });
}
jdavidbakr commented 3 years ago

I considered adding the migration but decided it would be better to let users add a local migration themselves as they need it. As you discovered, various ways of having it set up will cause complexities - this is why I delayed so long in making this change in the first place.

vesper8 commented 3 years ago

@jdavidbakr that's fair. I think it would have been nice to mention this in the upgrade guide and possibly to add an optional flag to the mail-tracker:migrate-recipients command so that it removes those unneeded columns after the migration is performed. I'm likely not the only one who's going to run into this issue