search

fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Improved Migrations #843

Closed danharper closed 12 years ago

danharper commented 12 years ago

(As discussed here: http://fuelphp.com/forums/topics/view/2644)

The current convention of adding a number to each new migration (eg. 001, 002, 003 etc.) does not work well at all on multi-user teams working on several feature branches at once.

For example, branch master is at revision 012. Steve and I both branch off to work on a feature. My feature requires 3 new migrations, and Steve's 2. When we go to merge these branches back into master, our migrations can conflict as both Steve and I have migrations numbered 013 and 014.

An overhaul of this system to perhaps use timestamps instead (or however other MVC frameworks do it, ie. Rails) should be on the roadmap for a 2.0 release.

I'd be happy to assist in working on something as we have recently switched to Fuel where I work.

jschreuder commented 12 years ago

Isn't that already supported: https://github.com/fuel/core/blob/1.1/develop/classes/migrate.php#L195

All I see is that it requires numeric, they don't need to be numbered consecutively.

philsturgeon commented 12 years ago

Yep still been too busy to see what oil is generating. If its time stamp then yay.

Emailing from my iPhone 4S like a BOSS!

On 22 Feb 2012, at 23:26, Jelmer Schreuderreply@reply.github.com wrote:

Isn't that already supported: https://github.com/fuel/core/blob/1.1/develop/classes/migrate.php#L195

All I see is that it requires numeric, they don't need to be numbered consecutively.

Reply to this email directly or view it on GitHub: https://github.com/fuel/core/issues/843#issuecomment-4127202

jschreuder commented 12 years ago

@danharper could you let us know if this is what you meant? In that case we can close this...

danharper commented 12 years ago

Yeah, it seems migrations may run fine with timestamps as migrate just sorts them numerically. But the current migration is stored in a config file as a number, eg. 003 so Fuel will not run migrations merged in from another branch if they have an older number.

Imagine this scenario:

Dan and Steve both have the latest master branch with migration 003_create_users. They both work on new features in their own branches.

Dan creates 2 new migrations, 004_create_tasks and 005_add_content_to_tasks. Steve creates one, 004_add_email_to_users.

They complete their work and merge branches back into master. Assuming they have their own environment migration config file, Dan's config says he's on migration 005. Steve's says he's on 004.

Dan tries to run migrations to get Steve's 004_add_email_to_users but Fuel says he's already on the latest version - 005.

Steve tries to run migrations and gets an error as Fuel looks for migrations above his current 004 and tries to run 005_add_content_to_tasks, but Fuel did not run Dan's 004_create_tasks.

RoR solves this problem (as far as I can tell) by using timestamps instead of numbers and storing all migrations, along with their 'migrated' status, in the database. When you run rake db:migrate, it collects all the migration files, and runs the ones not marked as 'migrated' in the database.

philsturgeon commented 12 years ago

Feel free to finish that up with a pull request! :)

Emailing from my iPhone 4S like a BOSS!

On 23 Feb 2012, at 11:25, Dan Harperreply@reply.github.com wrote:

Yeah, it seems migrations may fine with timestamps as migrate just sorts them numerically. But the current migration is stored in a config file as just a number, eg. 003 so Fuel will not run migrations merged in from another branch if they have an older number.

Imagine this scenario:

Dan and Steve both have the latest master branch with migration 003_create_users. They both work on new features in their own branches.

Dan creates 2 new migrations, 004_create_tasks and 005_add_content_to_tasks. Steve creates one, 004_add_email_to_users.

They complete their work and merge branches back into master. Assuming they have their own environment migration config file, Dan's config says he's on migration 005. Steve's says he's on 004.

Dan tries to run migrations to get Steve's 004_add_email_to_users but Fuel says he's already on the latest version - 005.

Steve tries to run migrations and gets an error as Fuel looks for migrations above his current 004 and tries to run 005_add_content_to_tasks, but Fuel did not run Dan's 004_create_tasks.

RoR solves this problem (as far as I can tell) by using timestamps instead of numbers and storing all migrations, along with their 'migrated' status, in the database. When you run rake db:migrate, it collects all the migration files, and runs the ones not marked as 'migrated' in the database.

Reply to this email directly or view it on GitHub: https://github.com/fuel/core/issues/843#issuecomment-4134688

WanWizard commented 12 years ago

A system like this can not be implemented without breaking BC, as it will require changes to both the config file array structure and the migrations table structure.

It also means the concept of "version" will disappear, which also means you can no longer migrate to or from a specific version number.

We really need to think how this should be implemented.

@philsturgeon, @jschreuder, any thoughts on this?

jschreuder commented 12 years ago

I already started implementing something along these lines for 2.0 (it's in the fuelphp/core repo). I don't think this can be done right in the 1.x branch without some major rewriting. Might be better to backport the new 2.0 code into a package available for 1.x.

philsturgeon commented 12 years ago

The version only works with current() while most people use latest() which is "use all available migrations".

We can just block the use of current, or alias it to latest anyway. It's not a big deal. latest() will now run all migrations that haven't been run, in order of their original time.

Phil Sturgeon

On Friday, 16 March 2012 at 13:32, Jelmer Schreuder wrote:

I already started implementing something along these lines for 2.0 (it's in the fuelphp/core repo). I don't think this can be done right in the 1.x branch without some major rewriting. Might be better to backport the new 2.0 code into a package available for 1.x.

Reply to this email directly or view it on GitHub: https://github.com/fuel/core/issues/843#issuecomment-4539625

WanWizard commented 12 years ago

I already have the code to migrate from a v1.0/v1.1 migration system to something that tracks individual migrations.

I'll check the 2.0 code to make sure the v1.2 format will be compatible and I'll drop the functionality to migrate to a specific version (since there is no sequence anymore).

WanWizard commented 12 years ago

@jschreuder, I see that 2.0 doesn't define the storage mechanism yet, so I'll just implement one for 1.2.

jschreuder commented 12 years ago

No Qb yet for 2.0, thus nothing to implement it with. code is mostly proof of concept so be careful, there might be some nasty bugs in there still.

WanWizard commented 12 years ago

Besides current(), up and down also no longer have meaning...

philsturgeon commented 12 years ago

Meh, kill em.

Phil Sturgeon

On Friday, 16 March 2012 at 16:18, Harro Verton wrote:

Besides current(), up and down also no longer have meaning...

Reply to this email directly or view it on GitHub: https://github.com/fuel/core/issues/843#issuecomment-4542799

tomschlick commented 12 years ago

basically with the changes proposed, up() and down() would become apply() and revert()

i am both for it for many reason and against it for the reasons @WanWizard stated. It give you increased flexibility for teams that have multiple migrations in development at one time but you lose the ability to have a specific database "state".

Maybe we allow both, a configurable option to follow a linear path or the other is the "stateless" approach in which migrations can have dependencies as we talked about a few weeks ago in IRC.

WanWizard commented 12 years ago

@tomschlick,

For the moment up() and down() are still there, I didn't want to change too much. The version parameter is also still there, but now defines the timestamp (or any other value you use to order the migrations). Combined with up() and down() you can migrate up (or down).

Migrations are now tracked individually, so when you merge migration files in between migrations already run, they will still be picked up.

tomschlick commented 12 years ago

@WanWizard awesome!

Btw, I wasn't suggesting changing the method names, just explaining what function those actually mimic with the new code to anyone who wasn't familiar with what all this was about.