statamic / eloquent-driver

Provides support for storing your Statamic data in a database, rather than flat files.
https://statamic.dev/tips/storing-content-in-a-database
MIT License
104 stars 71 forks source link

Migrations are publishable, they don't need to be loaded #255

Closed duncanmcclean closed 4 months ago

duncanmcclean commented 4 months ago

This pull request attempts to fix an issue identified in the comments of my previous PR, where the Eloquent Driver's migrations were being loaded, even though they had already been published and the tables already existed.

Since the filenames of migrations changed in #253, Laravel thought the migrations it was loading directly from the package's database/migrations directory were new migrations so it attempted to run them causing an error.

To make it easier to understand, here's what lead to the error happening:

  1. Using the Eloquent Driver v3.2 or below, publish its migrations w/ php artisan vendor:publish
    • The migrations will have been published into database/migrations.
    • The filenames for each of the migrations will have included the date & time the migrations were published.
  2. Then, update the Eloquent Driver to v3.3.0
    • This release contains changes to the way migrations were being published (#253).
    • Instead of using the current date & time in the migration filenames, a fixed date & time was used instead, to prevent issues where the same migrations could be published multiple times if you ran the vendor:publish command multiple times.
  3. After updating, attempt to run php artisan migrate
    • Upon running this command, you will have received an error about tables already existing.
    • For context: when running the migrate command, Laravel not only looks at your project's database/migrations directory but it also looks at any "migration paths" registered by packages (it's what the loadMigrationsFrom method does).
    • And because the Eloquent Driver was loading its migrations path, Laravel looked at its database/migrations directory and due to the filename changes made in #253, it thought they were new migrations so it attempted to run them, only to cause an error because they had already been published & run.

Since we recommend developers publish the migrations anyway, there's no issue in us stopping to load migrations, so this PR does that by removing the line which calls loadMigrationsFrom.

I've compared this to how Laravel Pulse, a first-party Laravel package, handles migrations. They just let developers publish the migrations instead of "forcibly" loading them.