[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

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', ''),

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,6 +237,7 @@ 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'	=> '',
@@ -242,9 +245,7 @@ class phpbb_db_migrator
 				'migration_end_time'	=> 0,
 			);
 
-		$depends = $migration->depends_on();
+		foreach ($state['migration_depends_on'] as $depend)
-
-		foreach ($depends as $depend)
 		{
 			if (!isset($this->migration_state[$depend]) ||
 				!$this->migration_state[$depend]['migration_schema_done'] ||
@@ -271,6 +272,8 @@ class phpbb_db_migrator
 			$state['migration_schema_done'] = true;
 		}
 		else
+		{
+			try
 			{
 				$result = $this->process_data_step($migration->update_data(), $state['migration_data_state']);
 
@@ -278,9 +281,20 @@ class phpbb_db_migrator
 				$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);
 
+				// 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 (!empty($state['migration_depends_on']) && in_array($migration, $state['migration_depends_on']))
-
-			if (in_array($migration, $migration_class->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 **/
+				// rethrow the exception
-				var_dump($step);
+				throw $e;
-				echo $e;
-				die();
 			}
 		}
 
@@ -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];
 	}
 
 	/**

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,

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 ,

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,

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,

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 '' ,

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,

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 '',

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', ''),
@@ -105,8 +106,18 @@ $migrator->load_migrations($phpbb_root_path . 'includes/db/migration/data/');
 $safe_time_limit = (ini_get('max_execution_time') / 2);
 
 while (!$migrator->finished())
+{
+	try
 	{
 		$migrator->update();
+	}
+	catch (phpbb_db_migration_exception $e)
+	{
+		echo $e;
+
+		garbage_collection();
+		exit_handler();
+	}
 
 	echo $migrator->last_run_migration['name'] . '<br />';
 

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>

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)),
+		);
+	}
+}

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_state('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_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_state('phpbb_dbal_migration_revert') !== false);
-		$this->assertTrue($this->migrator->migration_installed('phpbb_dbal_migration_revert_with_dependency'));
+		$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_state('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_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
+			while (!$this->migrator->finished())
-			$this->assertSqlResultEquals(
+			{
-				false,
+				$this->migrator->update();
-				"SELECT bar_column FROM phpbb_config WHERE config_name = 'foo'",
+			}
-				'Revert did not remove bar_column.'
+		}
-			);
+		catch (phpbb_db_migration_exception $e) {}
-		}
+
-		catch (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.');
+		}
 	}
 }