Skip to content

Conversation

@duncanmcclean
Copy link
Member

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 (Prevent migrations being published multiple times #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 Prevent migrations being published multiple times #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.

@duncanmcclean duncanmcclean marked this pull request as ready for review March 8, 2024 10:39
@duncanmcclean duncanmcclean merged commit 36ac24d into master Mar 8, 2024
@duncanmcclean duncanmcclean deleted the migrations-dont-need-to-be-loaded branch March 8, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant