Added support for timestamp-based migrations #1949

Merged
merged 8 commits into from Nov 13, 2012

Conversation

Projects
None yet
5 participants

Signed-off-by: Jonathon Hill jhill@brandmovers.com

Jonathon Hill Added support for timestamp-based migrations
Signed-off-by: Jonathon Hill <jhill@brandmovers.com>
34c8b9c
Contributor

narfbg commented Nov 5, 2012

Why is timestamp the default configuration value (especially with it being a new feature) and what are the benefits from this? Is having a different version number representation so important?

Good question. This will not break applications built on CodeIgniter < 3.0, since the library defaults to sequential if the setting is not defined in config/migration.php, which is the previous behavior. The reason the setting in the config file has been added with a default value of timestamp is to encourage the use of timestamp migrations for new applications, since it is a better method.

The reason this is better is because it makes it much easier to prevent conflicts when working in a team environment, or in an HMVC scenario where you're incorporating modules (or sparks) that need to modify the database schema.

A similar change took place in Ruby on Rails 2.1, as explained on their migrations page:

Internally Rails only uses the migration’s number (the timestamp) to identify them. Prior to Rails 2.1 the migration number started at 1 and was incremented each time a migration was generated. With multiple developers it was easy for these to clash requiring you to rollback migrations and renumber them. With Rails 2.1+ this is largely avoided by using the creation time of the migration to identify them.

...

The combination of timestamps and recording which migrations have been run allows Rails to handle common situations that occur with multiple developers.

For example Alice adds migrations 20080906120000 and 20080906123000 and Bob adds 20080906124500 and runs it. Alice finishes her changes and checks in her migrations and Bob pulls down the latest changes. When Bob runs rake db:migrate, Rails knows that it has not run Alice’s two migrations so it executes the up method for each migration.

Of course this is no substitution for communication within the team. For example, if Alice’s migration removed a table that Bob’s migration assumed to exist, then trouble would certainly strike.

@GDmac GDmac and 2 others commented on an outdated diff Nov 11, 2012

system/libraries/Migration.php
// If the migrations table is missing, make it
if ( ! $this->db->table_exists($this->_migration_table))
{
$this->dbforge->add_field(array(
- 'version' => array('type' => 'INT', 'constraint' => 3),
+ 'version' => array('type' => 'BIGINT', 'constraint' => 3),
@GDmac

GDmac Nov 11, 2012

Contributor

shouldn't the constraint of 3 also be raised, to be able to work with timestamps

@compwright

compwright Nov 12, 2012

Yes, good catch. Thanks.

@ckdarby

ckdarby Nov 12, 2012

Contributor

Looking forward to this.
On Nov 12, 2012 8:24 AM, "Jonathon Hill" notifications@github.com wrote:

In system/libraries/Migration.php:

    // If the migrations table is missing, make it
    if ( ! $this->db->table_exists($this->_migration_table))
    {
        $this->dbforge->add_field(array(
  •           'version' => array('type' => 'INT', 'constraint' => 3),
    
  •           'version' => array('type' => 'BIGINT', 'constraint' => 3),
    

Yes, good catch. Thanks.


Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1949/files#r2097094.

Contributor

GDmac commented Nov 11, 2012

👍 +1 for support for timestamp based migrations.

a question i'm hoping you might have an answer to and/or are willing to look at:
Rolling back to start, i.e. going to version(0), generates an error.
(even though the down() method does seem to get processed).

A more general issue is that the current migration process seems rather fragile.
When a (halting) error occurs (e.g. query or db forge add_key fails) then the migration is left half finished.
The version isn't updated in the migration table, but already created tables remain.
Any thoughts on this?

Contributor

narfbg commented Nov 11, 2012

OK. However, you'll need to update the pull request with the latest changes from the repository and alter the code so that it complies with our styleguide: http://codeigniter.com/user_guide/general/styleguide.html (stuff like using && instead of AND, curly braces on new lines, etc.).

I'd also change the name of 'migration_style' to 'migration_type', but that's not that much important.

@narfbg narfbg commented on the diff Nov 11, 2012

system/libraries/Migration.php
@@ -158,113 +181,80 @@ public function __construct($config = array())
*/
public function version($target_version)
{
- $start = $current_version = $this->_get_version();
- $stop = $target_version;
-
+ $current_version = (int) $this->_get_version();
+ $target_version = (int) $target_version;
@narfbg

narfbg Nov 11, 2012

Contributor

Also, please don't do these casts.

@compwright

compwright Nov 12, 2012

Why not? The reason they are there is to achieve uniformity. Some methods were returning integers, and others were returning strings. When data types are not uniform, strict comparisons (===) fail.

@narfbg

narfbg Nov 12, 2012

Contributor

Because if they should've been done, the current migrations wouldn't use a 0-padded 3-digit format. I didn't design it and I wouldn't have done it in that manner, but it is pretty obvious nonetheless.

@compwright

compwright Nov 12, 2012

Sorry, but that statement didn't make any sense to me. If you can show me a good reason to change it, I'm happy to change it, but I need something more substantive than "it's obvious."

@narfbg

narfbg Nov 13, 2012

Contributor

In the same way, "why not" is not a reason enough to change working code (which is my primary concern).
Not worth the argument anyway ...

@GDmac regarding your questions:

Rolling back to start, i.e. going to version(0), seems to work fine for me, and I did test that specific case when I was working on this PR.

Obviously for rolling back to version 0 to work, two conditions must be satisfied: a) your database schema should be in the correct state for the current migration (no partially applied migrations), and b) your migrations should have correct, working rollback code in the down() method.

If you are using InnoDB constraints, then there is a lot of little gotchas you should be aware of when performing migrations.

A more general issue is that the current migration process seems rather fragile.

This is a very difficult problem to solve, partly because MySQL puts each ALTER statement into its own transaction. One way to solve it is to copy the entire table, perform the migration on the copy, if it succeeds drop the original table and rename the copy. If it fails, then simply drop the copy.

However, if you do this on a live site without putting it in maintenance mode, you run a high risk of losing data which comes to your site during the migration process.

My recommendation is to perform schema changes with your migration system to begin with, so that you know you have tested each and every migration. Testing can prevent this from happening in most cases, and in the rare cases that it fails, you can just roll back the partial migration by hand.

You could also catch errors in your migration using a try { ... } catch () { ... } block, and handle rolling back the partial migration manually. This is a lot of extra work, though.

Jonathon Hill added some commits Nov 12, 2012

Jonathon Hill Fixed a mismatched constraint value when creating the migration table
Signed-off-by: Jonathon Hill <jhill@brandmovers.com>
275cf27
Jonathon Hill Merge remote-tracking branch 'upstream/develop' into develop
Conflicts:
	user_guide_src/source/changelog.rst

Signed-off-by: Jonathon Hill <jhill@brandmovers.com>
3978fc3
Jonathon Hill Changed the `migration_style` config setting to `migration_type`
Signed-off-by: Jonathon Hill <jhill@brandmovers.com>
b719bfd

@narfbg I didn't see any style guide violations. Did you see something I missed?

@narfbg narfbg and 1 other commented on an outdated diff Nov 12, 2012

system/libraries/Migration.php
return FALSE;
}
+
+ $previous = $number;
+
+ // Run migrations that are inside the target range
+ if (
+ ($method === 'up' AND $number > $current_version AND $number <= $target_version) OR
+ ($method === 'down' AND $number <= $current_version AND $number > $target_version)
+ ) {
@narfbg

narfbg Nov 12, 2012

Contributor

Curly brace needs to be on the next line, alone.

Also each occurence of AND must be replaced with &&.

@compwright

compwright Nov 12, 2012

The brace in this case was deliberate, but whatever (PSR-2: "When the ending list (whether or arguments or variables) is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them."). The CodeIgniter style guide doesn't address this.

@narfbg narfbg and 1 other commented on an outdated diff Nov 12, 2012

user_guide_src/source/changelog.rst
@@ -249,6 +250,9 @@ Release Date: Not Released
- Removed previously deprecated ``sha1()`` method.
- Changed :doc:`Language Library <libraries/language>` method ``load()`` to filter the language name with ``ctype_digit()``.
- :doc:`Profiler Library <general/profiling>` now also displays database object names.
+ - :doc:`Migration Library <libraries/migration>` changes include:
+ - Added support for timestamp-based migrations (enabled by default)
+ - Added ``$config['migration_style']`` to allow switching between sequential migrations and timestamp migrations
@narfbg

narfbg Nov 12, 2012

Contributor

'migration_type' here as well :)

@compwright

compwright Nov 12, 2012

Good catch, thanks.

Jonathon Hill added some commits Nov 12, 2012

Jonathon Hill Fixed a typo
Signed-off-by: Jonathon Hill <jhill@brandmovers.com>
49c5eda
Jonathon Hill Code style fixes
Signed-off-by: Jonathon Hill <jhill@brandmovers.com>
02ea66e

@narfbg narfbg and 1 other commented on an outdated diff Nov 12, 2012

system/libraries/Migration.php
@@ -126,12 +140,21 @@ public function __construct($config = array())
{
show_error('Migrations configuration file (migration.php) must have "migration_table" set.');
}
+
+ // Migration basename regex
+ $this->_migration_regex = $this->_migration_type === 'timestamp' ? '/^\d{14}_(\w+)$/' : '/^\d{3}_(\w+)$/';
@narfbg

narfbg Nov 12, 2012

Contributor

Parenthesis around the condition here would make it a bit more readable, moving the values to the next line even more:

$this->_migration_regex = ($this->_migration_type === 'timestamp')
    ? '/^\d{14}_(\w+)$/' : '/^\d{3}_(\w+)$/';

@narfbg narfbg and 1 other commented on an outdated diff Nov 12, 2012

user_guide_src/source/changelog.rst
@@ -40,6 +40,7 @@ Release Date: Not Released
- Updated support for csv files in mimes.php.
- Added some more doctypes.
- Added Romanian, Greek, Vietnamese and Cyrilic characters in *application/config/foreign_characters.php*.
+ - Added Romanian, Greek and Vietnamese characters in *foreign_characters.php*.
@narfbg

narfbg Nov 12, 2012

Contributor

This here should go ...

Jonathon Hill added some commits Nov 12, 2012

Jonathon Hill Improved code readability 4ddf944
Jonathon Hill Fixed an issue with my merge
Signed-off-by: Jonathon Hill <jhill@brandmovers.com>
93f989b

@narfbg narfbg added a commit that referenced this pull request Nov 13, 2012

@narfbg narfbg Merge pull request #1949 from compwright/develop
Added support for timestamp-based migrations
5d5bc05

@narfbg narfbg merged commit 5d5bc05 into bcit-ci:develop Nov 13, 2012

1 check passed

default The Travis build passed
Details
Contributor

narfbg commented Nov 13, 2012

@compwright Congrats. :)

Just a reply to your comment on braces and the style guide. It does address them: http://codeigniter.com/user_guide/general/styleguide.html#code_indenting

@narfbg narfbg added a commit that referenced this pull request Nov 13, 2012

@narfbg narfbg Clean-up following PR #1949 39eb806

Thanks guys for your help!

qu1j0t3 commented Mar 29, 2013

The current timestamp migration code breaks on 32-bit PHP, as far as I can tell. I have created a pull request to address it.

Contributor

GDmac commented Mar 29, 2013

to comment on my 5 month old comment.
A more general issue is that the current migration process seems rather fragile

Would it be helpful to update the migration version first and then do processing? Because if the new version is set, any downward drop table if exists is much less error-prone then checks on checks upwards about what tables and data where created / merged and all. That way, if an up fails, you could simply issue a down, fix the migration and retry the up. (just some thoughts)

I actually think that is a great idea, but it should be the subject of a separate pull request. You could take a stab at it and see if it gets accepted!

@nonchip nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

@narfbg narfbg Merge pull request #1949 from compwright/develop
Added support for timestamp-based migrations
29e773d

@nonchip nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

@narfbg narfbg Clean-up following PR #1949 7f74a95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment