From 2c878ead311b8b71cabb4070ce848d5b84470ce8 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Mon, 3 Feb 2014 15:06:43 -0600 Subject: [PATCH 1/3] [ticket/11880] Break up schema changes in the migrator PHPBB3-11880 --- phpBB/phpbb/db/migrator.php | 107 ++++++++++++--- tests/dbal/migration/schema.php | 33 +++++ tests/dbal/migrator_test.php | 20 +++ tests/migrator/get_schema_steps_test.php | 163 +++++++++++++++++++++++ 4 files changed, 302 insertions(+), 21 deletions(-) create mode 100644 tests/dbal/migration/schema.php create mode 100644 tests/migrator/get_schema_steps_test.php diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index 8186493800..7dc113204e 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/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->apply_schema_changes($migration->update_schema()); - $state['migration_schema_done'] = true; + $this->last_run_migration['task'] = 'process_schema_step'; + $steps = self::get_schema_steps($migration->update_schema()); + $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 . " - WHERE migration_name = '" . $this->db->sql_escape($name) . "'"; - $this->db->sql_query($sql); + $state['migration_data_state'] = ($result === true) ? '' : $result; + $state['migration_schema_done'] = ($result === true) ? false : true; - 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; + } } diff --git a/tests/dbal/migration/schema.php b/tests/dbal/migration/schema.php new file mode 100644 index 0000000000..46cf66eaec --- /dev/null +++ b/tests/dbal/migration/schema.php @@ -0,0 +1,33 @@ + 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', + ), + ), + ); + } +} diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index c6b4c289d3..9a6f33523c 100644 --- a/tests/dbal/migrator_test.php +++ b/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')); + } } diff --git a/tests/migrator/get_schema_steps_test.php b/tests/migrator/get_schema_steps_test.php new file mode 100644 index 0000000000..c37d55ea40 --- /dev/null +++ b/tests/migrator/get_schema_steps_test.php @@ -0,0 +1,163 @@ + 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)); + } +} From 38eb6b0e135293d51cc100937a6072b32e214c34 Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Wed, 5 Feb 2014 09:57:36 -0600 Subject: [PATCH 2/3] [ticket/11880] Move get_schema_steps function to a migrator helper class PHPBB3-11880 --- phpBB/config/migrator.yml | 4 + phpBB/phpbb/db/migrator.php | 74 ++----------------- tests/dbal/migrator_test.php | 3 +- tests/extension/manager_test.php | 3 +- tests/extension/metadata_manager_test.php | 3 +- tests/migrator/get_schema_steps_test.php | 9 ++- .../phpbb_functional_test_case.php | 3 +- 7 files changed, 27 insertions(+), 72 deletions(-) diff --git a/phpBB/config/migrator.yml b/phpBB/config/migrator.yml index a94609418f..202421c09f 100644 --- a/phpBB/config/migrator.yml +++ b/phpBB/config/migrator.yml @@ -10,6 +10,10 @@ services: - %core.php_ext% - %core.table_prefix% - @migrator.tool_collection + - @migrator.helper + + migrator.helper: + class: phpbb\db\migration\helper migrator.tool_collection: class: phpbb\di\service_collection diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index 7dc113204e..4fb3f1a241 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -25,6 +25,9 @@ class migrator /** @var \phpbb\db\tools */ protected $db_tools; + /** @var \phpbb\db\migration\helper */ + protected $helper; + /** @var string */ protected $table_prefix; @@ -65,11 +68,12 @@ class migrator /** * Constructor of the database migrator */ - public function __construct(\phpbb\config\config $config, \phpbb\db\driver\driver $db, \phpbb\db\tools $db_tools, $migrations_table, $phpbb_root_path, $php_ext, $table_prefix, $tools) + public function __construct(\phpbb\config\config $config, \phpbb\db\driver\driver $db, \phpbb\db\tools $db_tools, $migrations_table, $phpbb_root_path, $php_ext, $table_prefix, $tools, \phpbb\db\migration\helper $helper) { $this->config = $config; $this->db = $db; $this->db_tools = $db_tools; + $this->helper = $helper; $this->migrations_table = $migrations_table; @@ -232,7 +236,7 @@ class migrator if (!$state['migration_schema_done']) { $this->last_run_migration['task'] = 'process_schema_step'; - $steps = self::get_schema_steps($migration->update_schema()); + $steps = $this->helper->get_schema_steps($migration->update_schema()); $result = $this->process_data_step($steps, $state['migration_data_state']); $state['migration_data_state'] = ($result === true) ? '' : $result; @@ -336,7 +340,7 @@ class migrator } else if ($state['migration_schema_done']) { - $steps = self::get_schema_steps($migration->revert_schema()); + $steps = $this->helper->get_schema_steps($migration->revert_schema()); $result = $this->process_data_step($steps, $state['migration_data_state']); $state['migration_data_state'] = ($result === true) ? '' : $result; @@ -750,68 +754,4 @@ 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; - } } diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 9a6f33523c..c075f19825 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -50,7 +50,8 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case dirname(__FILE__) . '/../../phpBB/', 'php', 'phpbb_', - $tools + $tools, + new \phpbb\db\migration\helper() ); $container = new phpbb_mock_container_builder(); diff --git a/tests/extension/manager_test.php b/tests/extension/manager_test.php index cc32a6af4e..789dc20d14 100644 --- a/tests/extension/manager_test.php +++ b/tests/extension/manager_test.php @@ -106,7 +106,8 @@ class phpbb_extension_manager_test extends phpbb_database_test_case $phpbb_root_path, $php_ext, $table_prefix, - array() + array(), + new \phpbb\db\migration\helper() ); $container = new phpbb_mock_container_builder(); $container->set('migrator', $migrator); diff --git a/tests/extension/metadata_manager_test.php b/tests/extension/metadata_manager_test.php index 592421f9e7..a3c4cc89e9 100644 --- a/tests/extension/metadata_manager_test.php +++ b/tests/extension/metadata_manager_test.php @@ -62,7 +62,8 @@ class phpbb_extension_metadata_manager_test extends phpbb_database_test_case $this->phpbb_root_path, 'php', $this->table_prefix, - array() + array(), + new \phpbb\db\migration\helper() ); $container = new phpbb_mock_container_builder(); $container->set('migrator', $migrator); diff --git a/tests/migrator/get_schema_steps_test.php b/tests/migrator/get_schema_steps_test.php index c37d55ea40..f354d3617f 100644 --- a/tests/migrator/get_schema_steps_test.php +++ b/tests/migrator/get_schema_steps_test.php @@ -9,6 +9,13 @@ class get_schema_steps_test extends phpbb_test_case { + public function setUp() + { + parent::setUp(); + + $this->helper = new \phpbb\db\migration\helper(); + } + public function schema_provider() { return array( @@ -158,6 +165,6 @@ class get_schema_steps_test extends phpbb_test_case */ public function test_get_schema_steps($schema_changes, $expected) { - $this->assertEquals($expected, \phpbb\db\migrator::get_schema_steps($schema_changes)); + $this->assertEquals($expected, $this->helper->get_schema_steps($schema_changes)); } } diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php index 71d03746a9..55f9cdb947 100644 --- a/tests/test_framework/phpbb_functional_test_case.php +++ b/tests/test_framework/phpbb_functional_test_case.php @@ -194,7 +194,8 @@ class phpbb_functional_test_case extends phpbb_test_case $phpbb_root_path, $php_ext, self::$config['table_prefix'], - array() + array(), + new \phpbb\db\migration\helper() ); $container = new phpbb_mock_container_builder(); $container->set('migrator', $migrator); From be28445b66c8c74f92a50b086fd6b472ecaa963b Mon Sep 17 00:00:00 2001 From: Nathan Guse Date: Wed, 5 Feb 2014 10:00:01 -0600 Subject: [PATCH 3/3] [ticket/11880] migration helper file PHPBB3-11880 --- phpBB/phpbb/db/migration/helper.php | 82 +++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 phpBB/phpbb/db/migration/helper.php diff --git a/phpBB/phpbb/db/migration/helper.php b/phpBB/phpbb/db/migration/helper.php new file mode 100644 index 0000000000..28a6a56511 --- /dev/null +++ b/phpBB/phpbb/db/migration/helper.php @@ -0,0 +1,82 @@ + 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; + } +}