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 75 forks source link

Migration update could potentially cause duplicate issues #181

Closed phadaphunk closed 1 year ago

phadaphunk commented 1 year ago

This was added recently

class AddBlueprintToEntriesTable extends UpdateScript
{
    public function shouldUpdate($newVersion, $oldVersion)
    {
        return ! Schema::hasColumn(config('statamic.eloquent-driver.table_prefix', '').'entries', 'blueprint');
    }

Now if someone pulls our repo and composer update before running the migration, a new migration (duplicate) will be created because he does not yet have this column. This could lead to someone potentially pushing duplicate migrations to a server and making it crash on deploy...

image

Shouldn't the update check look for the migration file in the project instead of looking if the column already exists?

ryanmitchell commented 1 year ago

Would it not make more sense for you to just not git commit the migration?

phadaphunk commented 1 year ago

@ryanmitchell

All of our migrations are source controlled, and we wouldn't want to start manually picking which should be source controlled and which should not, since all the other ones have always worked fine.

ryanmitchell commented 1 year ago

Then surely you run into this issue with every migration from this repo? eg the one in 2.3.0

phadaphunk commented 1 year ago

@ryanmitchell Probably the first time someone actually updates and pushes without migrating locally which created the duplicate.

ryanmitchell commented 1 year ago

I'm not really sure how you want us to solve this (or if we even should)? If you have a suggestion I'll happily review a PR for it as long as it doesn't affect other users of this repo.

The only thing I can think off the top of my head is adding a config variable to prevent migrations being published (and update scripts being run at all). But this is a really niche use case.

maxi032 commented 1 year ago

wrapp it in a

if (!Schema::hasColumn('entries', 'blueprint')){ 

}

?

ryanmitchell commented 1 year ago

@maxi032 yes that would work but again the issue is bigger. If migrations are being applied properly there would be no need to do that.

To ‘solve’ this we would need to apply similar to logic to all past and figure migrations which makes no sense to me.