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

Prevent migrations being published multiple times #253

Closed duncanmcclean closed 4 months ago

duncanmcclean commented 4 months ago

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:publish command 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 publishesMigrations method (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 updates though) and also moved the migration code into a new publishMigrations method to tidy things up a little.

ryanmitchell commented 4 months ago

Seems good. Do you want it merged or is the plan to hold off for v5?

duncanmcclean commented 4 months ago

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 😄

marvinosswald commented 4 months ago

This causes issues with existing installations as my system now tries to migrate all of those again as the name changed from 2024_02_07... to 2024_03_07... am i overseeing something i should do differently ?

potsky commented 4 months ago

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 migrations table with this PostgreSQL query:

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 2024_01_09 date with your installation date of course!

@duncanmcclean Can you confirm we need to do this?

duncanmcclean commented 4 months ago

When are the migrations being re-published? When you run composer update?

marvinosswald commented 4 months ago

https://github.com/statamic/eloquent-driver/pull/253/files#diff-abf1fa77a19f720052b75bc802df0260157b9064d8c3c12a80794f01e9acc065L216

Doesn't need to be republished for this problem. this is the problematic line, this makes laravel "discover" the migrations without them being published afaik.

duncanmcclean commented 4 months ago

Doesn't need to be republished for this problem. this is the problematic line, this makes laravel "discover" the migrations without them being published afaik.

That line is only related to loading the migrations within the test suite, it won't affect "real" applications.

duncanmcclean commented 4 months ago

Ah, I've figured it out...

In addition to letting you publish the migrations yourself into your database/migrations directory, the Eloquent Driver is also directly loading the migration files from it's database/migrations directory, which leads to errors when running php artisan migrate.

I've opened a PR which fixes it (#255), which I'll tag a release for shortly.

duncanmcclean commented 4 months ago

It should hopefully be fixed in v3.3.1. Thanks!