romanzipp / Laravel-Queue-Monitor

Monitoring Laravel Jobs with your Database
https://packagist.org/packages/romanzipp/laravel-queue-monitor
MIT License
660 stars 91 forks source link

Remove Eloquent usage in UpdateQueueMonitorTable migration #138

Closed sgtlambda closed 9 months ago

sgtlambda commented 9 months ago

This PR updates the UpdateQueueMonitorTable migration to use DB::table everywhere rather than Eloquent to select existing items in the \UpdateQueueMonitorTable::upgradeColumns method.

It's usually preferable to avoid referencing Eloquent models during migrations - the definition, fields, methods etc. of the model may change over time such that it no longer aligns with the state of the database as of when the migration was introduced.

Additionally, since the Monitor model may in fact be configured to use a specific connection, using it breaks the migration when for whatever reason the migration needs to run using a different connection -- note that DB::table()->update() on line 48 will always use the current default connection (which is, in my opinion, the desired behaviour), so there was a fundamental inconsistency there regardless;

Updating the migration as proposed in the PR fixed a problem for me in a multitenant application that runs database migration tests in a separate (temporary) database upon every release. I have the Monitor model configured to use the central database in the context of an application that has a central database and several tenant-specific databases, where the queue_monitor table goes in the central database.

romanzipp commented 9 months ago

I actually never thought about this, but it makes sense - nice change. Thank you!

romanzipp commented 9 months ago

Released in 5.0.1