[ticket/11880] Break up schema changes in the migrator

PHPBB3-11880

phpBB/phpbb/db/migrator.php

@@ -83,6 +83,8 @@ class migrator
 			$this->tools[$tool->get_name()] = $tool;
 		}
 
+		$this->tools['dbtools'] = $this->db_tools;
+
 		$this->load_migration_state();
 	}
 
@@ -229,9 +231,12 @@ class migrator
 
 		if (!$state['migration_schema_done'])
 		{
-			$this->last_run_migration['task'] = 'apply_schema_changes';
+			$this->last_run_migration['task'] = 'process_schema_step';
-			$this->apply_schema_changes($migration->update_schema());
+			$steps = self::get_schema_steps($migration->update_schema());
-			$state['migration_schema_done'] = true;
+			$result = $this->process_data_step($steps, $state['migration_data_state']);
+
+			$state['migration_data_state'] = ($result === true) ? '' : $result;
+			$state['migration_schema_done'] = ($result === true);
 		}
 		else if (!$state['migration_data_done'])
 		{
@@ -329,32 +334,27 @@ class migrator
 
 			$this->set_migration_state($name, $state);
 		}
-		else
+		else if ($state['migration_schema_done'])
 		{
-			$this->apply_schema_changes($migration->revert_schema());
+			$steps = self::get_schema_steps($migration->revert_schema());
+			$result = $this->process_data_step($steps, $state['migration_data_state']);
 
-			$sql = 'DELETE FROM ' . $this->migrations_table . "
+			$state['migration_data_state'] = ($result === true) ? '' : $result;
-				WHERE migration_name = '" . $this->db->sql_escape($name) . "'";
+			$state['migration_schema_done'] = ($result === true) ? false : true;
-			$this->db->sql_query($sql);
 
-			unset($this->migration_state[$name]);
+			if (!$state['migration_schema_done'])
+			{
+				$sql = 'DELETE FROM ' . $this->migrations_table . "
+					WHERE migration_name = '" . $this->db->sql_escape($name) . "'";
+				$this->db->sql_query($sql);
+
+				unset($this->migration_state[$name]);
+			}
 		}
 
 		return true;
 	}
 
-	/**
-	* Apply schema changes from a migration
-	*
-	* Just calls db_tools->perform_schema_changes
-	*
-	* @param array $schema_changes from migration
-	*/
-	protected function apply_schema_changes($schema_changes)
-	{
-		$this->db_tools->perform_schema_changes($schema_changes);
-	}
-
 	/**
 	* Process the data step of the migration
 	*
@@ -498,6 +498,7 @@ class migrator
 
 				return $this->get_callable_from_step($step);
 			break;
+
 			case 'custom':
 				if (!is_callable($parameters[0]))
 				{
@@ -749,4 +750,68 @@ class migrator
 
 		return $this->migrations;
 	}
+
+	/**
+	 * Get the schema steps from an array of schema changes
+	 *
+	 * This splits up $schema_changes into individual changes so that the
+	 * changes can be chunked
+	 *
+	 * @param array $schema_changes from migration
+	 * @return array
+	 */
+	static public function get_schema_steps($schema_changes)
+	{
+		$steps = array();
+
+		// Nested level of data (only supports 1/2 currently)
+		$nested_level = array(
+			'drop_tables'		=> 1,
+			'add_tables'		=> 1,
+			'change_columns'	=> 2,
+			'add_columns'		=> 2,
+			'drop_keys'			=> 2,
+			'drop_columns'		=> 2,
+			'add_primary_keys'	=> 2, // perform_schema_changes only uses one level, but second is in the function
+			'add_unique_index'	=> 2,
+			'add_index'			=> 2,
+		);
+
+		foreach ($nested_level as $change_type => $data_depth)
+		{
+			if (!empty($schema_changes[$change_type]))
+			{
+				foreach ($schema_changes[$change_type] as $key => $value)
+				{
+					if ($data_depth === 1)
+					{
+						$steps[] = array(
+							'dbtools.perform_schema_changes', array(array(
+									$change_type	=> array(
+									$value,
+								),
+							)),
+						);
+					}
+					else if ($data_depth === 2)
+					{
+						foreach ($value as $key2 => $value2)
+						{
+							$steps[] = array(
+								'dbtools.perform_schema_changes', array(array(
+									$change_type	=> array(
+										$key => array(
+											$key2	=> $value2,
+										),
+									),
+								)),
+							);
+						}
+					}
+				}
+			}
+		}
+
+		return $steps;
+	}
 }

tests/dbal/migration/schema.php

@@ -0,0 +1,33 @@
+<?php
+/**
+*
+* @package testing
+* @copyright (c) 2014 phpBB Group
+* @license http://opensource.org/licenses/gpl-license.php GNU Public License
+*
+*/
+
+class phpbb_dbal_migration_schema extends \phpbb\db\migration\migration
+{
+	function update_schema()
+	{
+		return array(
+			'add_columns' => array(
+				$this->table_prefix . 'config' => array(
+					'test_column1' => array('BOOL', 1),
+				),
+			),
+		);
+	}
+
+	function revert_schema()
+	{
+		return array(
+			'drop_columns' => array(
+				$this->table_prefix . 'config' => array(
+					'test_column1',
+				),
+			),
+		);
+	}
+}

tests/dbal/migrator_test.php

@@ -16,6 +16,7 @@ require_once dirname(__FILE__) . '/migration/revert.php';
 require_once dirname(__FILE__) . '/migration/revert_with_dependency.php';
 require_once dirname(__FILE__) . '/migration/fail.php';
 require_once dirname(__FILE__) . '/migration/installed.php';
+require_once dirname(__FILE__) . '/migration/schema.php';
 
 class phpbb_dbal_migrator_test extends phpbb_database_test_case
 {
@@ -267,4 +268,23 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 			$this->fail('Installed test failed');
 		}
 	}
+
+	public function test_schema()
+	{
+		$this->migrator->set_migrations(array('phpbb_dbal_migration_schema'));
+
+		while (!$this->migrator->finished())
+		{
+			$this->migrator->update();
+		}
+
+		$this->assertTrue($this->db_tools->sql_column_exists('phpbb_config', 'test_column1'));
+
+		while ($this->migrator->migration_state('phpbb_dbal_migration_schema'))
+		{
+			$this->migrator->revert('phpbb_dbal_migration_schema');
+		}
+
+		$this->assertFalse($this->db_tools->sql_column_exists('phpbb_config', 'test_column1'));
+	}
 }

tests/migrator/get_schema_steps_test.php

@@ -0,0 +1,163 @@
+<?php
+/**
+*
+* @package testing
+* @copyright (c) 2014 phpBB Group
+* @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+*
+*/
+
+class get_schema_steps_test extends phpbb_test_case
+{
+	public function schema_provider()
+	{
+		return array(
+			array(
+				array(
+					'add_tables'	=> array('table1', 'table2', 'table3'),
+					'drop_tables'	=> array('table1', 'table2', 'table3'),
+					'add_index'	=> array(
+						'table1'	=> array(
+							'index1'	=> 'column1',
+							'index2'	=> 'column2',
+						),
+						'table2'	=> array(
+							'index1'	=> 'column1',
+							'index2'	=> 'column2',
+						),
+					),
+					'add_columns'	=> array(
+						'table1'	=> array(
+							'column1'	=> array('foo'),
+							'column2'	=> array('bar'),
+						),
+					),
+					'change_columns'	=> array(
+						'table1'	=> array(
+							'column1'	=> array('foo'),
+							'column2'	=> array('bar'),
+						),
+					),
+					'drop_columns'	=> array(
+						'table1'	=> array(
+							'column1',
+							'column2',
+						),
+					),
+					'add_unique_index'	=> array(
+						'table1'	=> array(
+							'index1'	=> 'column1',
+							'index2'	=> 'column2',
+						),
+					),
+					'drop_keys'	=> array(
+						'table1'	=> array(
+							'column1',
+							'column2',
+						),
+					),
+					'add_primary_keys'	=> array(
+						'table1' => array('foo'),
+						'table2' => array('bar'),
+						'table3' => array('foobar'),
+					),
+				),
+				array(
+					array('dbtools.perform_schema_changes', array(array('drop_tables'	=> array('table1')))),
+					array('dbtools.perform_schema_changes', array(array('drop_tables'	=> array('table2')))),
+					array('dbtools.perform_schema_changes', array(array('drop_tables'	=> array('table3')))),
+					array('dbtools.perform_schema_changes', array(array('add_tables'	=> array('table1')))),
+					array('dbtools.perform_schema_changes', array(array('add_tables'	=> array('table2')))),
+					array('dbtools.perform_schema_changes', array(array('add_tables'	=> array('table3')))),
+					array('dbtools.perform_schema_changes', array(array('change_columns'	=> array(
+						'table1'	=> array(
+							'column1'	=> array('foo'),
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('change_columns'	=> array(
+						'table1'	=> array(
+							'column2'	=> array('bar'),
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_columns'	=> array(
+						'table1'	=> array(
+							'column1'	=> array('foo'),
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_columns'	=> array(
+						'table1'	=> array(
+							'column2'	=> array('bar'),
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('drop_keys'	=> array(
+						'table1'	=> array(
+							0 => 'column1',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('drop_keys'	=> array(
+						'table1'	=> array(
+							1 => 'column2',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('drop_columns'	=> array(
+						'table1'	=> array(
+							0 => 'column1',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('drop_columns'	=> array(
+						'table1'	=> array(
+							1 => 'column2',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_primary_keys'	=> array(
+						'table1' => array('foo'),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_primary_keys'	=> array(
+						'table2' => array('bar'),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_primary_keys'	=> array(
+						'table3' => array('foobar'),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_unique_index'	=> array(
+						'table1'	=> array(
+							'index1'	=> 'column1',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_unique_index'	=> array(
+						'table1'	=> array(
+							'index2'	=> 'column2',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_index'	=> array(
+						'table1'	=> array(
+							'index1'	=> 'column1',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_index'	=> array(
+						'table1'	=> array(
+							'index2'	=> 'column2',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_index'	=> array(
+						'table2'	=> array(
+							'index1'	=> 'column1',
+						),
+					)))),
+					array('dbtools.perform_schema_changes', array(array('add_index'	=> array(
+						'table2'	=> array(
+							'index2'	=> 'column2',
+						),
+					)))),
+				),
+			),
+		);
+	}
+
+	/**
+	 * @dataProvider schema_provider
+	 */
+	public function test_get_schema_steps($schema_changes, $expected)
+	{
+		$this->assertEquals($expected, \phpbb\db\migrator::get_schema_steps($schema_changes));
+	}
+}