From 9ddf02243e37adb27984ef34e208943a413266b8 Mon Sep 17 00:00:00 2001 From: PayBas Date: Sun, 18 May 2014 15:55:28 +0200 Subject: [PATCH 1/5] [ticket/12561] Add "after" check to schema_generator for columns_add PHPBB3-12561 --- phpBB/phpbb/db/migration/schema_generator.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/phpBB/phpbb/db/migration/schema_generator.php b/phpBB/phpbb/db/migration/schema_generator.php index 5d40b0b26f..872430e078 100644 --- a/phpBB/phpbb/db/migration/schema_generator.php +++ b/phpBB/phpbb/db/migration/schema_generator.php @@ -107,7 +107,17 @@ class schema_generator { foreach ($add_columns as $column => $column_data) { - $this->tables[$table]['COLUMNS'][$column] = $column_data; + if (isset($column_data['after'])) + { + $columns = $this->tables[$table]['COLUMNS']; + $offset = array_search($column_data['after'], array_keys($columns)); + unset($column_data['after']); + $this->tables[$table]['COLUMNS'] = array_merge(array_slice($columns, 0, $offset + 1, true), array($column => array_values($column_data)), array_slice($columns, $offset)); + } + else + { + $this->tables[$table]['COLUMNS'][$column] = $column_data; + } } } } From 05839f85994c1281936d2dd64215c068e55b6d54 Mon Sep 17 00:00:00 2001 From: PayBas Date: Fri, 23 May 2014 22:24:57 +0200 Subject: [PATCH 2/5] [ticket/12561] Added test for "after" PHPBB3-12561 --- tests/dbal/migration/dummy_order.php | 31 ++++++++++++++++++++++++ tests/migrator/schema_generator_test.php | 17 +++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 tests/dbal/migration/dummy_order.php diff --git a/tests/dbal/migration/dummy_order.php b/tests/dbal/migration/dummy_order.php new file mode 100644 index 0000000000..011ac7c1e9 --- /dev/null +++ b/tests/dbal/migration/dummy_order.php @@ -0,0 +1,31 @@ + array( + $this->table_prefix . 'column_order_test' => array( + 'COLUMNS' => array( + 'foobar1' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + ), + 'PRIMARY_KEY' => array('foobar1'), + ), + ), + 'add_columns' => array( + $this->table_prefix . 'column_order_test' => array( + 'foobar2' => array('BOOL', 0, 'after' => 'foobar1'), + ), + ), + ); + } +} diff --git a/tests/migrator/schema_generator_test.php b/tests/migrator/schema_generator_test.php index 4bac447229..4de6064895 100644 --- a/tests/migrator/schema_generator_test.php +++ b/tests/migrator/schema_generator_test.php @@ -7,6 +7,8 @@ * */ +require_once __DIR__ . '/../dbal/migration/dummy_order.php'; + class schmema_generator_test extends phpbb_test_case { public function setUp() @@ -46,4 +48,19 @@ class schmema_generator_test extends phpbb_test_case $this->assertArrayHasKey('phpbb_users', $this->generator->get_schema()); } + + public function test_check_column_position_success() + { + $this->get_schema_generator(array( + 'phpbb_dbal_migration_dummy_order', + )); + + $tables = $this->generator->get_schema(); + $columns = $tables[$this->table_prefix . 'column_order_test']['COLUMNS']; + + $offset1 = array_search('foobar1', array_keys($columns)); + $offset2 = array_search('foobar2', array_keys($columns)); + + $this->assertEquals($offset1 + 1, $offset2, 'The schema generator could not position the column correctly, using the "after" option in the migration script.'); + } } From c51b926631eb8e339c7a29cf3ff145e9e158b04f Mon Sep 17 00:00:00 2001 From: PayBas Date: Mon, 26 May 2014 19:28:37 +0200 Subject: [PATCH 3/5] [ticket/12561] Add check to see if "after" column actually exists If not, just append to the end PHPBB3-12561 --- phpBB/phpbb/db/migration/schema_generator.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/phpBB/phpbb/db/migration/schema_generator.php b/phpBB/phpbb/db/migration/schema_generator.php index 872430e078..f36e6a96d5 100644 --- a/phpBB/phpbb/db/migration/schema_generator.php +++ b/phpBB/phpbb/db/migration/schema_generator.php @@ -112,7 +112,15 @@ class schema_generator $columns = $this->tables[$table]['COLUMNS']; $offset = array_search($column_data['after'], array_keys($columns)); unset($column_data['after']); - $this->tables[$table]['COLUMNS'] = array_merge(array_slice($columns, 0, $offset + 1, true), array($column => array_values($column_data)), array_slice($columns, $offset)); + + if ($offset == false) + { + $this->tables[$table]['COLUMNS'][$column] = array_values($column_data); + } + else + { + $this->tables[$table]['COLUMNS'] = array_merge(array_slice($columns, 0, $offset + 1, true), array($column => array_values($column_data)), array_slice($columns, $offset)); + } } else { From 3cabe5fd792fced4120de02cfef66900b49516c4 Mon Sep 17 00:00:00 2001 From: PayBas Date: Mon, 26 May 2014 20:47:19 +0200 Subject: [PATCH 4/5] [ticket/12561] Added tests for "after last", "after missing" and "empty" Also removed tabs PHPBB3-12561 --- phpBB/phpbb/db/migration/schema_generator.php | 4 +- tests/dbal/migration/dummy_order.php | 49 +++++++++++++- tests/migrator/schema_generator_test.php | 67 +++++++++++++++++-- 3 files changed, 111 insertions(+), 9 deletions(-) diff --git a/phpBB/phpbb/db/migration/schema_generator.php b/phpBB/phpbb/db/migration/schema_generator.php index f36e6a96d5..18c99fb94a 100644 --- a/phpBB/phpbb/db/migration/schema_generator.php +++ b/phpBB/phpbb/db/migration/schema_generator.php @@ -112,8 +112,8 @@ class schema_generator $columns = $this->tables[$table]['COLUMNS']; $offset = array_search($column_data['after'], array_keys($columns)); unset($column_data['after']); - - if ($offset == false) + + if ($offset === false) { $this->tables[$table]['COLUMNS'][$column] = array_values($column_data); } diff --git a/tests/dbal/migration/dummy_order.php b/tests/dbal/migration/dummy_order.php index 011ac7c1e9..af047eb416 100644 --- a/tests/dbal/migration/dummy_order.php +++ b/tests/dbal/migration/dummy_order.php @@ -13,7 +13,7 @@ class phpbb_dbal_migration_dummy_order extends \phpbb\db\migration\migration { return array( 'add_tables' => array( - $this->table_prefix . 'column_order_test' => array( + $this->table_prefix . 'column_order_test1' => array( 'COLUMNS' => array( 'foobar1' => array('BOOL', 0), 'foobar3' => array('BOOL', 0), @@ -22,10 +22,55 @@ class phpbb_dbal_migration_dummy_order extends \phpbb\db\migration\migration ), ), 'add_columns' => array( - $this->table_prefix . 'column_order_test' => array( + $this->table_prefix . 'column_order_test1' => array( 'foobar2' => array('BOOL', 0, 'after' => 'foobar1'), ), ), + + 'add_tables' => array( + $this->table_prefix . 'column_order_test2' => array( + 'COLUMNS' => array( + 'foobar1' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + ), + 'PRIMARY_KEY' => array('foobar1'), + ), + ), + 'add_columns' => array( + $this->table_prefix . 'column_order_test2' => array( + 'foobar4' => array('BOOL', 0, 'after' => 'foobar3'), + ), + ), + + 'add_tables' => array( + $this->table_prefix . 'column_order_test3' => array( + 'COLUMNS' => array( + 'foobar1' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + ), + 'PRIMARY_KEY' => array('foobar1'), + ), + ), + 'add_columns' => array( + $this->table_prefix . 'column_order_test3' => array( + 'foobar5' => array('BOOL', 0, 'after' => 'non-existing'), + ), + ), + + 'add_tables' => array( + $this->table_prefix . 'column_order_test4' => array( + 'COLUMNS' => array( + 'foobar1' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + ), + 'PRIMARY_KEY' => array('foobar1'), + ), + ), + 'add_columns' => array( + $this->table_prefix . 'column_order_test4' => array( + 'foobar5' => array('BOOL', 0, 'after' => ''), + ), + ), ); } } diff --git a/tests/migrator/schema_generator_test.php b/tests/migrator/schema_generator_test.php index 4de6064895..c87c4cb904 100644 --- a/tests/migrator/schema_generator_test.php +++ b/tests/migrator/schema_generator_test.php @@ -49,18 +49,75 @@ class schmema_generator_test extends phpbb_test_case $this->assertArrayHasKey('phpbb_users', $this->generator->get_schema()); } - public function test_check_column_position_success() + protected $expected_results_between = array( + 'foobar1' => array('BOOL', 0), + 'foobar2' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + ); + + public function test_check_column_position_between_success() { $this->get_schema_generator(array( 'phpbb_dbal_migration_dummy_order', )); $tables = $this->generator->get_schema(); - $columns = $tables[$this->table_prefix . 'column_order_test']['COLUMNS']; + $columns = $tables[$this->table_prefix . 'column_order_test1']['COLUMNS']; - $offset1 = array_search('foobar1', array_keys($columns)); - $offset2 = array_search('foobar2', array_keys($columns)); + $this->assertEquals($columns, $expected_results_between, 'The schema generator could not position the column correctly between column 1 and 3, using the "after" option in the migration script.'); + } - $this->assertEquals($offset1 + 1, $offset2, 'The schema generator could not position the column correctly, using the "after" option in the migration script.'); + protected $expected_results_after_last = array( + 'foobar1' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + 'foobar4' => array('BOOL', 0), + ); + + public function test_check_column_position_after_last_success() + { + $this->get_schema_generator(array( + 'phpbb_dbal_migration_dummy_order', + )); + + $tables = $this->generator->get_schema(); + $columns = $tables[$this->table_prefix . 'column_order_test2']['COLUMNS']; + + $this->assertEquals($columns, $expected_results_after_last, 'The schema generator could not position the column correctly after the last column, using the "after" option in the migration script.'); + } + + protected $expected_results_after_missing = array( + 'foobar1' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + 'foobar5' => array('BOOL', 0), + ); + + public function test_check_column_position_after_missing_success() + { + $this->get_schema_generator(array( + 'phpbb_dbal_migration_dummy_order', + )); + + $tables = $this->generator->get_schema(); + $columns = $tables[$this->table_prefix . 'column_order_test3']['COLUMNS']; + + $this->assertEquals($columns, $expected_results_after_missing, 'The schema generator could not position the column after a "missing" column value, using the "after" option in the migration script.'); + } + + protected $expected_results_after_empty = array( + 'foobar1' => array('BOOL', 0), + 'foobar3' => array('BOOL', 0), + 'foobar5' => array('BOOL', 0), + ); + + public function test_check_column_position_after_empty_success() + { + $this->get_schema_generator(array( + 'phpbb_dbal_migration_dummy_order', + )); + + $tables = $this->generator->get_schema(); + $columns = $tables[$this->table_prefix . 'column_order_test4']['COLUMNS']; + + $this->assertEquals($columns, $expected_results_after_empty, 'The schema generator could not position the column after an "empty" column value, using the "after" option in the migration script.'); } } From 9f37d414be1cc13377b9af1777220577c7af645c Mon Sep 17 00:00:00 2001 From: PayBas Date: Tue, 27 May 2014 00:41:11 +0200 Subject: [PATCH 5/5] [ticket/12561] Reworked tests by nickvergessen PHPBB3-12561 --- tests/dbal/migration/dummy_order.php | 50 --------- tests/dbal/migration/dummy_order_0.php | 22 ++++ tests/dbal/migration/dummy_order_1.php | 22 ++++ tests/dbal/migration/dummy_order_2.php | 22 ++++ tests/dbal/migration/dummy_order_3.php | 22 ++++ tests/dbal/migration/dummy_order_4.php | 22 ++++ tests/dbal/migration/dummy_order_5.php | 23 ++++ tests/migrator/schema_generator_test.php | 133 ++++++++++++----------- 8 files changed, 205 insertions(+), 111 deletions(-) create mode 100644 tests/dbal/migration/dummy_order_0.php create mode 100644 tests/dbal/migration/dummy_order_1.php create mode 100644 tests/dbal/migration/dummy_order_2.php create mode 100644 tests/dbal/migration/dummy_order_3.php create mode 100644 tests/dbal/migration/dummy_order_4.php create mode 100644 tests/dbal/migration/dummy_order_5.php diff --git a/tests/dbal/migration/dummy_order.php b/tests/dbal/migration/dummy_order.php index af047eb416..b8590f5074 100644 --- a/tests/dbal/migration/dummy_order.php +++ b/tests/dbal/migration/dummy_order.php @@ -21,56 +21,6 @@ class phpbb_dbal_migration_dummy_order extends \phpbb\db\migration\migration 'PRIMARY_KEY' => array('foobar1'), ), ), - 'add_columns' => array( - $this->table_prefix . 'column_order_test1' => array( - 'foobar2' => array('BOOL', 0, 'after' => 'foobar1'), - ), - ), - - 'add_tables' => array( - $this->table_prefix . 'column_order_test2' => array( - 'COLUMNS' => array( - 'foobar1' => array('BOOL', 0), - 'foobar3' => array('BOOL', 0), - ), - 'PRIMARY_KEY' => array('foobar1'), - ), - ), - 'add_columns' => array( - $this->table_prefix . 'column_order_test2' => array( - 'foobar4' => array('BOOL', 0, 'after' => 'foobar3'), - ), - ), - - 'add_tables' => array( - $this->table_prefix . 'column_order_test3' => array( - 'COLUMNS' => array( - 'foobar1' => array('BOOL', 0), - 'foobar3' => array('BOOL', 0), - ), - 'PRIMARY_KEY' => array('foobar1'), - ), - ), - 'add_columns' => array( - $this->table_prefix . 'column_order_test3' => array( - 'foobar5' => array('BOOL', 0, 'after' => 'non-existing'), - ), - ), - - 'add_tables' => array( - $this->table_prefix . 'column_order_test4' => array( - 'COLUMNS' => array( - 'foobar1' => array('BOOL', 0), - 'foobar3' => array('BOOL', 0), - ), - 'PRIMARY_KEY' => array('foobar1'), - ), - ), - 'add_columns' => array( - $this->table_prefix . 'column_order_test4' => array( - 'foobar5' => array('BOOL', 0, 'after' => ''), - ), - ), ); } } diff --git a/tests/dbal/migration/dummy_order_0.php b/tests/dbal/migration/dummy_order_0.php new file mode 100644 index 0000000000..e45bb0ad44 --- /dev/null +++ b/tests/dbal/migration/dummy_order_0.php @@ -0,0 +1,22 @@ + array( + $this->table_prefix . 'column_order_test1' => array( + 'foobar2' => array('BOOL', 0, 'after' => 'foobar1'), + ), + ), + ); + } +} diff --git a/tests/dbal/migration/dummy_order_1.php b/tests/dbal/migration/dummy_order_1.php new file mode 100644 index 0000000000..73f043af5d --- /dev/null +++ b/tests/dbal/migration/dummy_order_1.php @@ -0,0 +1,22 @@ + array( + $this->table_prefix . 'column_order_test1' => array( + 'foobar4' => array('BOOL', 0, 'after' => 'foobar3'), + ), + ), + ); + } +} diff --git a/tests/dbal/migration/dummy_order_2.php b/tests/dbal/migration/dummy_order_2.php new file mode 100644 index 0000000000..1483b98050 --- /dev/null +++ b/tests/dbal/migration/dummy_order_2.php @@ -0,0 +1,22 @@ + array( + $this->table_prefix . 'column_order_test1' => array( + 'foobar5' => array('BOOL', 0, 'after' => 'non-existing'), + ), + ), + ); + } +} diff --git a/tests/dbal/migration/dummy_order_3.php b/tests/dbal/migration/dummy_order_3.php new file mode 100644 index 0000000000..79c542e088 --- /dev/null +++ b/tests/dbal/migration/dummy_order_3.php @@ -0,0 +1,22 @@ + array( + $this->table_prefix . 'column_order_test1' => array( + 'foobar6' => array('BOOL', 0, 'after' => ''), + ), + ), + ); + } +} diff --git a/tests/dbal/migration/dummy_order_4.php b/tests/dbal/migration/dummy_order_4.php new file mode 100644 index 0000000000..229a120ecf --- /dev/null +++ b/tests/dbal/migration/dummy_order_4.php @@ -0,0 +1,22 @@ + array( + $this->table_prefix . 'column_order_test1' => array( + 'foobar7' => array('BOOL', 0), + ), + ), + ); + } +} diff --git a/tests/dbal/migration/dummy_order_5.php b/tests/dbal/migration/dummy_order_5.php new file mode 100644 index 0000000000..04d755009d --- /dev/null +++ b/tests/dbal/migration/dummy_order_5.php @@ -0,0 +1,23 @@ + array( + $this->table_prefix . 'column_order_test1' => array( + 'foobar8' => array('BOOL', 0, 'after' => 'foobar3'), + 'foobar9' => array('BOOL', 0, 'after' => 'foobar3'), + ), + ), + ); + } +} diff --git a/tests/migrator/schema_generator_test.php b/tests/migrator/schema_generator_test.php index c87c4cb904..14b423f276 100644 --- a/tests/migrator/schema_generator_test.php +++ b/tests/migrator/schema_generator_test.php @@ -8,9 +8,18 @@ */ require_once __DIR__ . '/../dbal/migration/dummy_order.php'; +require_once __DIR__ . '/../dbal/migration/dummy_order_0.php'; +require_once __DIR__ . '/../dbal/migration/dummy_order_1.php'; +require_once __DIR__ . '/../dbal/migration/dummy_order_2.php'; +require_once __DIR__ . '/../dbal/migration/dummy_order_3.php'; +require_once __DIR__ . '/../dbal/migration/dummy_order_4.php'; +require_once __DIR__ . '/../dbal/migration/dummy_order_5.php'; -class schmema_generator_test extends phpbb_test_case +class schema_generator_test extends phpbb_test_case { + /** @var \phpbb\db\migration\schema_generator */ + protected $generator; + public function setUp() { parent::setUp(); @@ -49,75 +58,77 @@ class schmema_generator_test extends phpbb_test_case $this->assertArrayHasKey('phpbb_users', $this->generator->get_schema()); } - protected $expected_results_between = array( - 'foobar1' => array('BOOL', 0), - 'foobar2' => array('BOOL', 0), - 'foobar3' => array('BOOL', 0), - ); - - public function test_check_column_position_between_success() + public function column_add_after_data() { - $this->get_schema_generator(array( - 'phpbb_dbal_migration_dummy_order', - )); - - $tables = $this->generator->get_schema(); - $columns = $tables[$this->table_prefix . 'column_order_test1']['COLUMNS']; - - $this->assertEquals($columns, $expected_results_between, 'The schema generator could not position the column correctly between column 1 and 3, using the "after" option in the migration script.'); + return array( + array( + 'phpbb_dbal_migration_dummy_order_0', + array( + 'foobar1', + 'foobar2', + 'foobar3', + ), + ), + array( + 'phpbb_dbal_migration_dummy_order_1', + array( + 'foobar1', + 'foobar3', + 'foobar4', + ), + ), + array( + 'phpbb_dbal_migration_dummy_order_2', + array( + 'foobar1', + 'foobar3', + 'foobar5', + ), + ), + array( + 'phpbb_dbal_migration_dummy_order_3', + array( + 'foobar1', + 'foobar3', + 'foobar6', + ), + ), + array( + 'phpbb_dbal_migration_dummy_order_4', + array( + 'foobar1', + 'foobar3', + 'foobar7', + ), + ), + array( + 'phpbb_dbal_migration_dummy_order_5', + array( + 'foobar1', + 'foobar3', + 'foobar9', + 'foobar8', + ), + ), + ); } - protected $expected_results_after_last = array( - 'foobar1' => array('BOOL', 0), - 'foobar3' => array('BOOL', 0), - 'foobar4' => array('BOOL', 0), - ); - - public function test_check_column_position_after_last_success() + /** + * @dataProvider column_add_after_data + */ + public function test_column_add_after($migration, $expected) { $this->get_schema_generator(array( 'phpbb_dbal_migration_dummy_order', + $migration, )); $tables = $this->generator->get_schema(); - $columns = $tables[$this->table_prefix . 'column_order_test2']['COLUMNS']; - $this->assertEquals($columns, $expected_results_after_last, 'The schema generator could not position the column correctly after the last column, using the "after" option in the migration script.'); - } - - protected $expected_results_after_missing = array( - 'foobar1' => array('BOOL', 0), - 'foobar3' => array('BOOL', 0), - 'foobar5' => array('BOOL', 0), - ); - - public function test_check_column_position_after_missing_success() - { - $this->get_schema_generator(array( - 'phpbb_dbal_migration_dummy_order', - )); - - $tables = $this->generator->get_schema(); - $columns = $tables[$this->table_prefix . 'column_order_test3']['COLUMNS']; - - $this->assertEquals($columns, $expected_results_after_missing, 'The schema generator could not position the column after a "missing" column value, using the "after" option in the migration script.'); - } - - protected $expected_results_after_empty = array( - 'foobar1' => array('BOOL', 0), - 'foobar3' => array('BOOL', 0), - 'foobar5' => array('BOOL', 0), - ); - - public function test_check_column_position_after_empty_success() - { - $this->get_schema_generator(array( - 'phpbb_dbal_migration_dummy_order', - )); - - $tables = $this->generator->get_schema(); - $columns = $tables[$this->table_prefix . 'column_order_test4']['COLUMNS']; - - $this->assertEquals($columns, $expected_results_after_empty, 'The schema generator could not position the column after an "empty" column value, using the "after" option in the migration script.'); + $this->assertEquals( + $expected, + array_keys($tables[$this->table_prefix . 'column_order_test1']['COLUMNS']), + 'The schema generator could not position the column correctly, using the "after" option in the migration script.' + ); } }