[feature/migrations] Store depends on in the database (serialized)

This is required so that when migrations are reverted we can check through all installed migrations and make sure that all dependencies are handled properly and so that we are only required to load the migrations files that could be dependent on the ones installed. I believe in normal proper use the old way might have worked, but in case something happens and an unrelated migration file is installed, but cannot be loaded, this makes sure we do not stop everything unless we absolutely must (one of those files is dependent on something we want to revert). PHPBB3-9737
author: Nathan Guse <nathaniel.guse@gmail.com> 2013-01-10 15:09:51 -0600
committer: Nathan Guse <nathaniel.guse@gmail.com> 2013-01-10 15:09:51 -0600
commit: d50500860fe44a78c8f29e0f2382b96da17c0b62 (patch)
tree: 77d9b2691d02fc77fd786ae28cae5cba4ef1084b
parent: dbe71bb170dc684311174bb025696c81f1d50883 (diff)
download: forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar
forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar.gz
forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar.bz2
forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar.xz
forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.zip
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) {}
 	}
 }
author	Nathan Guse <nathaniel.guse@gmail.com>	2013-01-10 15:09:51 -0600
committer	Nathan Guse <nathaniel.guse@gmail.com>	2013-01-10 15:09:51 -0600
commit	d50500860fe44a78c8f29e0f2382b96da17c0b62 (patch)
tree	77d9b2691d02fc77fd786ae28cae5cba4ef1084b
parent	dbe71bb170dc684311174bb025696c81f1d50883 (diff)
download	forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar.gz forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar.bz2 forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.tar.xz forums-d50500860fe44a78c8f29e0f2382b96da17c0b62.zip