13 files changed, 157 insertions, 48 deletions
diff --git a/phpBB/develop/create_schema_files.php b/phpBB/develop/create_schema_files.php
index b37dcc246f..d0864b975d 100644
--- a/phpBB/develop/create_schema_files.php
+++ b/phpBB/develop/create_schema_files.php
@@ -1276,6 +1276,7 @@ function get_schema_struct()
 	$schema_data['phpbb_migrations'] = array(
 		'COLUMNS'		=> array(
 			'migration_name'			=> array('VCHAR', ''),
+			'migration_depends_on'		=> array('TEXT', ''),
 			'migration_schema_done'		=> array('BOOL', 0),
 			'migration_data_done'		=> array('BOOL', 0),
 			'migration_data_state'		=> array('TEXT', ''),
diff --git a/phpBB/includes/db/migrator.php b/phpBB/includes/db/migrator.php
index 13114e7df1..2ec44a5a45 100644
--- a/phpBB/includes/db/migrator.php
+++ b/phpBB/includes/db/migrator.php
@@ -106,6 +106,8 @@ class phpbb_db_migrator
 		while ($migration = $this->db->sql_fetchrow($result))
 		{
 			$this->migration_state[$migration['migration_name']] = $migration;
+
+			$this->migration_state[$migration['migration_name']]['migration_depends_on'] = unserialize($migration['migration_depends_on']);
 		}
 
 		$this->db->sql_freeresult($result);
@@ -235,16 +237,15 @@ class phpbb_db_migrator
 		$state = (isset($this->migration_state[$name])) ?
 			$this->migration_state[$name] :
 			array(
+				'migration_depends_on'	=> $migration->depends_on(),
 				'migration_schema_done' => false,
-				'migration_data_done' => false,
-				'migration_data_state' => '',
-				'migration_start_time' => 0,
-				'migration_end_time' => 0,
+				'migration_data_done'	=> false,
+				'migration_data_state'	=> '',
+				'migration_start_time'	=> 0,
+				'migration_end_time'	=> 0,
 			);
 
-		$depends = $migration->depends_on();
-
-		foreach ($depends as $depend)
+		foreach ($state['migration_depends_on'] as $depend)
 		{
 			if (!isset($this->migration_state[$depend]) ||
 				!$this->migration_state[$depend]['migration_schema_done'] ||
@@ -272,15 +273,28 @@ class phpbb_db_migrator
 		}
 		else
 		{
-			$result = $this->process_data_step($migration->update_data(), $state['migration_data_state']);
+			try
+			{
+				$result = $this->process_data_step($migration->update_data(), $state['migration_data_state']);
+
+				$state['migration_data_state'] = ($result === true) ? '' : $result;
+				$state['migration_data_done'] = ($result === true);
+				$state['migration_end_time'] = ($result === true) ? time() : 0;
+			}
+			catch (phpbb_db_migration_exception $e)
+			{
+				// Revert the schema changes
+				$this->revert($name);
 
-			$state['migration_data_state'] = ($result === true) ? '' : $result;
-			$state['migration_data_done'] = ($result === true);
-			$state['migration_end_time'] = ($result === true) ? time() : 0;
+				// Rethrow exception
+				throw $e;
+			}
 		}
 
+		$insert = $state;
+		$insert['migration_depends_on'] = serialize($state['migration_depends_on']);
 		$sql = 'UPDATE ' . $this->migrations_table . '
-			SET ' . $this->db->sql_build_array('UPDATE', $state) . "
+			SET ' . $this->db->sql_build_array('UPDATE', $insert) . "
 			WHERE migration_name = '" . $this->db->sql_escape($name) . "'";
 		$this->db->sql_query($sql);
 
@@ -292,8 +306,9 @@ class phpbb_db_migrator
 	/**
 	* Runs a single revert step from the last migration installed
 	*
+	* YOU MUST ADD/SET ALL MIGRATIONS THAT COULD BE DEPENDENT ON THE MIGRATION TO REVERT TO BEFORE CALLING THIS METHOD!
 	* The revert step can either be a schema or a (partial) data revert. To
-	* check if revert() needs to be called again use the migration_installed() method.
+	* check if revert() needs to be called again use the migration_state() method.
 	*
 	* @param string $migration String migration name to revert (including any that depend on this migration)
 	* @return null
@@ -306,12 +321,9 @@ class phpbb_db_migrator
 			return;
 		}
 
-		// Iterate through all installed migrations and make sure any dependencies are removed first
 		foreach ($this->migration_state as $name => $state)
 		{
-			$migration_class = $this->get_migration($name);
-
-			if (in_array($migration, $migration_class->depends_on()))
+			if (!empty($state['migration_depends_on']) && in_array($migration, $state['migration_depends_on']))
 			{
 				$this->revert($name);
 			}
@@ -358,8 +370,10 @@ class phpbb_db_migrator
 				$state['migration_data_done'] = ($result === true) ? false : true;
 			}
 
+			$insert = $state;
+			$insert['migration_depends_on'] = serialize($state['migration_depends_on']);
 			$sql = 'UPDATE ' . $this->migrations_table . '
-				SET ' . $this->db->sql_build_array('UPDATE', $state) . "
+				SET ' . $this->db->sql_build_array('UPDATE', $insert) . "
 				WHERE migration_name = '" . $this->db->sql_escape($name) . "'";
 			$this->db->sql_query($sql);
 
@@ -436,22 +450,20 @@ class phpbb_db_migrator
 			catch (phpbb_db_migration_exception $e)
 			{
 				// We should try rolling back here
-				foreach ($steps as $reverse_step)
+				foreach ($steps as $reverse_step_identifier => $reverse_step)
 				{
-					// Reverse the step that was run
-					$result = $this->run_step($step, false, !$revert);
-
 					// If we've reached the current step we can break because we reversed everything that was run
-					if ($reverse_step === $step)
+					if ($reverse_step_identifier == $step_identifier)
 					{
 						break;
 					}
+
+					// Reverse the step that was run
+					$result = $this->run_step($reverse_step, false, !$revert);
 				}
 
-				/** TODO Revert Schema **/
-				var_dump($step);
-				echo $e;
-				die();
+				// rethrow the exception
+				throw $e;
 			}
 		}
 
@@ -588,6 +600,7 @@ class phpbb_db_migrator
 	{
 		$migration_row = $state;
 		$migration_row['migration_name'] = $name;
+		$migration_row['migration_depends_on'] = serialize($state['migration_depends_on']);
 
 		$sql = 'INSERT INTO ' . $this->migrations_table . '
 			' . $this->db->sql_build_array('INSERT', $migration_row);
@@ -660,19 +673,19 @@ class phpbb_db_migrator
 	}
 
 	/**
-	* Checks whether a migration is installed
+	* Gets a migration state (whether it is installed and to what extent)
 	*
 	* @param string $migration String migration name to check if it is installed
-	* @return bool Whether the migrations have been applied
+	* @return bool|array False if the migration has not at all been installed, array
 	*/
-	public function migration_installed($migration)
+	public function migration_state($migration)
 	{
-		if (isset($this->migration_state[$migration]))
+		if (!isset($this->migration_state[$migration]))
 		{
-			return true;
+			return false;
 		}
 
-		return false;
+		return $this->migration_state[$migration];
 	}
 
 	/**
diff --git a/phpBB/install/schemas/firebird_schema.sql b/phpBB/install/schemas/firebird_schema.sql
index 535cbb0df4..45024c4049 100644
--- a/phpBB/install/schemas/firebird_schema.sql
+++ b/phpBB/install/schemas/firebird_schema.sql
@@ -589,6 +589,7 @@ CREATE INDEX phpbb_moderator_cache_forum_id ON phpbb_moderator_cache(forum_id);;
 # Table: 'phpbb_migrations'
 CREATE TABLE phpbb_migrations (
 	migration_name VARCHAR(255) CHARACTER SET NONE DEFAULT '' NOT NULL,
+	migration_depends_on BLOB SUB_TYPE TEXT CHARACTER SET NONE DEFAULT '' NOT NULL,
 	migration_schema_done INTEGER DEFAULT 0 NOT NULL,
 	migration_data_done INTEGER DEFAULT 0 NOT NULL,
 	migration_data_state BLOB SUB_TYPE TEXT CHARACTER SET NONE DEFAULT '' NOT NULL,
diff --git a/phpBB/install/schemas/mssql_schema.sql b/phpBB/install/schemas/mssql_schema.sql
index 43b8f3e556..e261e73d73 100644
--- a/phpBB/install/schemas/mssql_schema.sql
+++ b/phpBB/install/schemas/mssql_schema.sql
@@ -721,6 +721,7 @@ GO
 */
 CREATE TABLE [phpbb_migrations] (
 	[migration_name] [varchar] (255) DEFAULT ('') NOT NULL ,
+	[migration_depends_on] [varchar] (8000) DEFAULT ('') NOT NULL ,
 	[migration_schema_done] [int] DEFAULT (0) NOT NULL ,
 	[migration_data_done] [int] DEFAULT (0) NOT NULL ,
 	[migration_data_state] [varchar] (8000) DEFAULT ('') NOT NULL ,
diff --git a/phpBB/install/schemas/mysql_40_schema.sql b/phpBB/install/schemas/mysql_40_schema.sql
index c46230744a..0f184d4d05 100644
--- a/phpBB/install/schemas/mysql_40_schema.sql
+++ b/phpBB/install/schemas/mysql_40_schema.sql
@@ -413,6 +413,7 @@ CREATE TABLE phpbb_moderator_cache (
 # Table: 'phpbb_migrations'
 CREATE TABLE phpbb_migrations (
 	migration_name varbinary(255) DEFAULT '' NOT NULL,
+	migration_depends_on blob NOT NULL,
 	migration_schema_done tinyint(1) UNSIGNED DEFAULT '0' NOT NULL,
 	migration_data_done tinyint(1) UNSIGNED DEFAULT '0' NOT NULL,
 	migration_data_state blob NOT NULL,
diff --git a/phpBB/install/schemas/mysql_41_schema.sql b/phpBB/install/schemas/mysql_41_schema.sql
index fa94598e9c..b7564ceb7c 100644
--- a/phpBB/install/schemas/mysql_41_schema.sql
+++ b/phpBB/install/schemas/mysql_41_schema.sql
@@ -413,6 +413,7 @@ CREATE TABLE phpbb_moderator_cache (
 # Table: 'phpbb_migrations'
 CREATE TABLE phpbb_migrations (
 	migration_name varchar(255) DEFAULT '' NOT NULL,
+	migration_depends_on text NOT NULL,
 	migration_schema_done tinyint(1) UNSIGNED DEFAULT '0' NOT NULL,
 	migration_data_done tinyint(1) UNSIGNED DEFAULT '0' NOT NULL,
 	migration_data_state text NOT NULL,
diff --git a/phpBB/install/schemas/oracle_schema.sql b/phpBB/install/schemas/oracle_schema.sql
index f8a9d5e1a6..7796c3a729 100644
--- a/phpBB/install/schemas/oracle_schema.sql
+++ b/phpBB/install/schemas/oracle_schema.sql
@@ -803,6 +803,7 @@ CREATE INDEX phpbb_moderator_cache_forum_id ON phpbb_moderator_cache (forum_id)
 */
 CREATE TABLE phpbb_migrations (
 	migration_name varchar2(255) DEFAULT '' ,
+	migration_depends_on clob DEFAULT '' ,
 	migration_schema_done number(1) DEFAULT '0' NOT NULL,
 	migration_data_done number(1) DEFAULT '0' NOT NULL,
 	migration_data_state clob DEFAULT '' ,
diff --git a/phpBB/install/schemas/postgres_schema.sql b/phpBB/install/schemas/postgres_schema.sql
index c976659f05..9fd81ec06a 100644
--- a/phpBB/install/schemas/postgres_schema.sql
+++ b/phpBB/install/schemas/postgres_schema.sql
@@ -577,6 +577,7 @@ CREATE INDEX phpbb_moderator_cache_forum_id ON phpbb_moderator_cache (forum_id);
 */
 CREATE TABLE phpbb_migrations (
 	migration_name varchar(255) DEFAULT '' NOT NULL,
+	migration_depends_on varchar(8000) DEFAULT '' NOT NULL,
 	migration_schema_done INT2 DEFAULT '0' NOT NULL CHECK (migration_schema_done >= 0),
 	migration_data_done INT2 DEFAULT '0' NOT NULL CHECK (migration_data_done >= 0),
 	migration_data_state varchar(8000) DEFAULT '' NOT NULL,
diff --git a/phpBB/install/schemas/sqlite_schema.sql b/phpBB/install/schemas/sqlite_schema.sql
index 31a6f715a0..9d6d648c00 100644
--- a/phpBB/install/schemas/sqlite_schema.sql
+++ b/phpBB/install/schemas/sqlite_schema.sql
@@ -401,6 +401,7 @@ CREATE INDEX phpbb_moderator_cache_forum_id ON phpbb_moderator_cache (forum_id);
 # Table: 'phpbb_migrations'
 CREATE TABLE phpbb_migrations (
 	migration_name varchar(255) NOT NULL DEFAULT '',
+	migration_depends_on text(65535) NOT NULL DEFAULT '',
 	migration_schema_done INTEGER UNSIGNED NOT NULL DEFAULT '0',
 	migration_data_done INTEGER UNSIGNED NOT NULL DEFAULT '0',
 	migration_data_state text(65535) NOT NULL DEFAULT '',
diff --git a/phpBB/test.php b/phpBB/test.php
index 7a2dafb7ef..6602aa8dcc 100644
--- a/phpBB/test.php
+++ b/phpBB/test.php
@@ -88,6 +88,7 @@ if (!$db_tools->sql_table_exists(MIGRATIONS_TABLE))
 	$db_tools->sql_create_table(MIGRATIONS_TABLE, array(
 		'COLUMNS'		=> array(
 			'migration_name'			=> array('VCHAR', ''),
+			'migration_depends_on'		=> array('TEXT', ''),
 			'migration_schema_done'		=> array('BOOL', 0),
 			'migration_data_done'		=> array('BOOL', 0),
 			'migration_data_state'		=> array('TEXT', ''),
@@ -106,7 +107,17 @@ $safe_time_limit = (ini_get('max_execution_time') / 2);
 
 while (!$migrator->finished())
 {
-	$migrator->update();
+	try
+	{
+		$migrator->update();
+	}
+	catch (phpbb_db_migration_exception $e)
+	{
+		echo $e;
+
+		garbage_collection();
+		exit_handler();
+	}
 
 	echo $migrator->last_run_migration['name'] . '<br />';
 
diff --git a/tests/dbal/fixtures/migrator.xml b/tests/dbal/fixtures/migrator.xml
index 1f9079c811..25be4d4129 100644
--- a/tests/dbal/fixtures/migrator.xml
+++ b/tests/dbal/fixtures/migrator.xml
@@ -2,6 +2,7 @@
 <dataset>
 	<table name="phpbb_migrations">
 		<column>migration_name</column>
+		<column>migration_depends_on</column>
 		<column>migration_schema_done</column>
 		<column>migration_data_done</column>
 		<column>migration_data_state</column>
@@ -9,6 +10,7 @@
 		<column>migration_end_time</column>
 		<row>
 			<value>installed_migration</value>
+			<value></value>
 			<value>1</value>
 			<value>1</value>
 			<value></value>
diff --git a/tests/dbal/migration/fail.php b/tests/dbal/migration/fail.php
new file mode 100644
index 0000000000..8b5c521e09
--- /dev/null
+++ b/tests/dbal/migration/fail.php
@@ -0,0 +1,46 @@
+<?php
+/**
+*
+* @package migration
+* @copyright (c) 2012 phpBB Group
+* @license http://opensource.org/licenses/gpl-license.php GNU Public License v2
+*
+*/
+
+class phpbb_dbal_migration_fail extends phpbb_db_migration
+{
+	function depends_on()
+	{
+		return array();
+	}
+
+	function update_schema()
+	{
+		return array(
+			'add_columns' => array(
+				$this->table_prefix . 'config' => array(
+					'test_column' => array('BOOL', 1),
+				),
+			),
+		);
+	}
+
+	function revert_schema()
+	{
+		return array(
+			'drop_columns' => array(
+				$this->table_prefix . 'config' => array(
+					'test_column',
+				),
+			),
+		);
+	}
+
+	function update_data()
+	{
+		return array(
+			array('config.add', array('foobar3', true)),
+			array('config.update', array('does_not_exist', true)),
+		);
+	}
+}
diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php
index 84bcb109b2..69db7ca047 100644
--- a/tests/dbal/migrator_test.php
+++ b/tests/dbal/migrator_test.php
@@ -18,6 +18,7 @@ require_once dirname(__FILE__) . '/migration/if.php';
 require_once dirname(__FILE__) . '/migration/recall.php';
 require_once dirname(__FILE__) . '/migration/revert.php';
 require_once dirname(__FILE__) . '/migration/revert_with_dependency.php';
+require_once dirname(__FILE__) . '/migration/fail.php';
 
 class phpbb_dbal_migrator_test extends phpbb_database_test_case
 {
@@ -163,8 +164,8 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 
 		$this->migrator->set_migrations(array('phpbb_dbal_migration_revert', 'phpbb_dbal_migration_revert_with_dependency'));
 
-		$this->assertFalse($this->migrator->migration_installed('phpbb_dbal_migration_revert'));
-		$this->assertFalse($this->migrator->migration_installed('phpbb_dbal_migration_revert_with_dependency'));
+		$this->assertFalse($this->migrator->migration_state('phpbb_dbal_migration_revert'));
+		$this->assertFalse($this->migrator->migration_state('phpbb_dbal_migration_revert_with_dependency'));
 
 		// Install the migration first
 		while (!$this->migrator->finished())
@@ -172,8 +173,8 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 			$this->migrator->update();
 		}
 
-		$this->assertTrue($this->migrator->migration_installed('phpbb_dbal_migration_revert'));
-		$this->assertTrue($this->migrator->migration_installed('phpbb_dbal_migration_revert_with_dependency'));
+		$this->assertTrue($this->migrator->migration_state('phpbb_dbal_migration_revert') !== false);
+		$this->assertTrue($this->migrator->migration_state('phpbb_dbal_migration_revert_with_dependency') !== false);
 
 		$this->assertSqlResultEquals(
 			array(array('bar_column' => '1')),
@@ -183,25 +184,53 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 
 		$this->assertTrue(isset($this->config['foobartest']));
 
-		while ($this->migrator->migration_installed('phpbb_dbal_migration_revert'))
+		while ($this->migrator->migration_state('phpbb_dbal_migration_revert') !== false)
 		{
 			$this->migrator->revert('phpbb_dbal_migration_revert');
 		}
 
-		$this->assertFalse($this->migrator->migration_installed('phpbb_dbal_migration_revert'));
-		$this->assertFalse($this->migrator->migration_installed('phpbb_dbal_migration_revert_with_dependency'));
+		$this->assertFalse($this->migrator->migration_state('phpbb_dbal_migration_revert'));
+		$this->assertFalse($this->migrator->migration_state('phpbb_dbal_migration_revert_with_dependency'));
 
 		$this->assertFalse(isset($this->config['foobartest']));
 
+		$sql = 'SELECT * FROM phpbb_config';
+		$result = $this->db->sql_query_limit($sql, 1);
+		$row = $this->db->sql_fetchrow($result);
+		$this->db->sql_freeresult($result);
+
+		if (isset($row['bar_column']))
+		{
+			$this->fail('Revert did not remove test_column.');
+		}
+	}
+
+	public function test_fail()
+	{
+		$this->migrator->set_migrations(array('phpbb_dbal_migration_fail'));
+
+		$this->assertFalse(isset($this->config['foobar3']));
+
 		try
 		{
-			// Should cause an error
-			$this->assertSqlResultEquals(
-				false,
-				"SELECT bar_column FROM phpbb_config WHERE config_name = 'foo'",
-				'Revert did not remove bar_column.'
-			);
+			while (!$this->migrator->finished())
+			{
+				$this->migrator->update();
+			}
+		}
+		catch (phpbb_db_migration_exception $e) {}
+
+		// Failure should have caused an automatic roll-back, so this should not exist.
+		$this->assertFalse(isset($this->config['foobar3']));
+
+		$sql = 'SELECT * FROM phpbb_config';
+		$result = $this->db->sql_query_limit($sql, 1);
+		$row = $this->db->sql_fetchrow($result);
+		$this->db->sql_freeresult($result);
+
+		if (isset($row['test_column']))
+		{
+			$this->fail('Revert did not remove test_column.');
 		}
-		catch (Exception $e) {}
 	}
 }