-
Notifications
You must be signed in to change notification settings - Fork 92
Prevent migrations being published multiple times #253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This allows them to be split a little easier in the tests.
|
Seems good. Do you want it merged or is the plan to hold off for v5? |
Yeah, let's get it merged, it'll allow for testing the Core PR. Just wanted to check it looked good for you before merging since I've haven't touched this package much 😄 |
|
This causes issues with existing installations as my system now tries to migrate all of those again as the name changed from |
|
Me too @marvinosswald. The migrations seem to be loaded from the package now. So I have removed all targeted migrations from our repository and have renamed the migrations in the UPDATE migrations
SET migration='2024_03_07_100000' || SUBSTRING (migration, '^2024_01_09_[0-9]{6}(_create.*$)')
WHERE migration LIKE '2024_01_09_%_create_%_table';Change the @duncanmcclean Can you confirm we need to do this? |
|
When are the migrations being re-published? When you run |
|
Doesn't need to be republished for this problem. |
That line is only related to loading the migrations within the test suite, it won't affect "real" applications. |
|
Ah, I've figured it out... In addition to letting you publish the migrations yourself into your I've opened a PR which fixes it (#255), which I'll tag a release for shortly. |
|
It should hopefully be fixed in v3.3.1. Thanks! |
Once statamic/eloquent-driver#253 has been merged, it won't be possible for the same migration to be published multiple times so this will no longer be an issue.
This pull request changes the way migrations are published so they use fixed timestamps, to prevent issues where the same migrations could be published multiple times.
When you run the
vendor:publishcommand and attempt to publish something which has already been published, Laravel will ignore publishing the file and give you a warning.However, in the case of the Eloquent driver, because our "destination filename" was dynamic based on the current date & time, the filename Laravel would check against would never match up to an existing migration. This could lead to the same migration being published multiple times.
While Laravel 11 does include a new
publishesMigrationsmethod (introduced in laravel/framework#49246), which replaces the timestamps in migration filenames, it has the same pitfall we were running into before in that it doesn't check for existence for past migrations.This PR adjusts how migrations are published so all the migrations have a fixed timestamp, rather than a dynamic one so Laravel can recognise any duplicates before publishing.
I adjusted the name of the migration files themselves (not the stubs in
updatesthough) and also moved the migration code into a newpublishMigrationsmethod to tidy things up a little.