From 9fb649793de65a598615c542861281ff15a60439 Mon Sep 17 00:00:00 2001 From: Oliver Schramm Date: Wed, 10 Aug 2016 14:32:00 +0200 Subject: [ticket/14742] Pause after each schema change It is certainly better than running them all at once PHPBB3-14742 --- phpBB/language/en/migrator.php | 1 + phpBB/phpbb/db/migrator.php | 13 +++++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/phpBB/language/en/migrator.php b/phpBB/language/en/migrator.php index 244a5faadf..7623636941 100644 --- a/phpBB/language/en/migrator.php +++ b/phpBB/language/en/migrator.php @@ -50,6 +50,7 @@ $lang = array_merge($lang, array( 'MIGRATION_NOT_FULFILLABLE' => 'The migration "%1$s" is not fulfillable, missing migration "%2$s".', 'MIGRATION_NOT_VALID' => '%s is not a valid migration.', 'MIGRATION_SCHEMA_DONE' => 'Installed Schema: %1$s; Time: %2$.2f seconds', + 'MIGRATION_SCHEMA_IN_PROGRESS' => 'Installing Schema: %1$s; Time: %2$.2f seconds', 'MIGRATION_SCHEMA_RUNNING' => 'Installing Schema: %s.', 'MIGRATION_INVALID_DATA_MISSING_CONDITION' => 'A migration is invalid. An if statement helper is missing a condition.', diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index 7fc3e787e2..739959d7b7 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -308,7 +308,14 @@ class migrator $state['migration_data_state'] = ($result === true) ? '' : $result; $state['migration_schema_done'] = ($result === true); - $this->output_handler->write(array('MIGRATION_SCHEMA_DONE', $name, $elapsed_time), migrator_output_handler_interface::VERBOSITY_NORMAL); + if ($state['migration_schema_done']) + { + $this->output_handler->write(array('MIGRATION_SCHEMA_DONE', $name, $elapsed_time), migrator_output_handler_interface::VERBOSITY_NORMAL); + } + else + { + $this->output_handler->write(array('MIGRATION_SCHEMA_IN_PROGRESS', $name, $elapsed_time), migrator_output_handler_interface::VERBOSITY_VERY_VERBOSE); + } } else if (!$state['migration_data_done']) { @@ -493,8 +500,10 @@ class migrator try { // Result will be null or true if everything completed correctly + // After any schema update step we allow to pause, since + // database changes can take quite some time $result = $this->run_step($step, $last_result, $revert); - if ($result !== null && $result !== true) + if ($result !== null && $result !== true && strpos($step[0], 'dbtools') !== 0) { return serialize(array( 'result' => $result, -- cgit v1.2.1 From 8e1461ca61e3f452935a1253d3afe65e7322d6bc Mon Sep 17 00:00:00 2001 From: Oliver Schramm Date: Wed, 10 Aug 2016 14:55:39 +0200 Subject: [ticket/14742] Avoid loop while reverting data This combines reverted updata_data and revert_data into a single array. PHPBB3-14742 --- phpBB/phpbb/db/migration/helper.php | 32 +++++++++++++++++++++++++++ phpBB/phpbb/db/migration/tool/config.php | 5 +++++ phpBB/phpbb/db/migration/tool/config_text.php | 5 +++++ phpBB/phpbb/db/migration/tool/module.php | 5 +++++ phpBB/phpbb/db/migration/tool/permission.php | 5 +++++ phpBB/phpbb/db/migrator.php | 23 +++++++++---------- 6 files changed, 63 insertions(+), 12 deletions(-) diff --git a/phpBB/phpbb/db/migration/helper.php b/phpBB/phpbb/db/migration/helper.php index e40deeb37b..bce2efff51 100644 --- a/phpBB/phpbb/db/migration/helper.php +++ b/phpBB/phpbb/db/migration/helper.php @@ -81,4 +81,36 @@ class helper return $steps; } + + /** + * Reverse the update steps from an array of data changes + * + * 'If' statements and custom methods will be skipped, for all + * other calls the reverse method of the tool class will be called + * + * @param array $steps Update changes from migration + * + * @return array + */ + public function reverse_update_data($steps) + { + $reversed_array = array(); + + foreach ($steps as $step) + { + $parts = explode('.', $step[0]); + $parameters = $step[1]; + + $class = $parts[0]; + $method = isset($parts[1]) ? $parts[1] : false; + + if ($class !== 'if' && $class !== 'custom') + { + array_unshift($parameters, $method); + $reversed_array[] = array($class . '.reverse', $parameters); + } + } + + return array_reverse($reversed_array); + } } diff --git a/phpBB/phpbb/db/migration/tool/config.php b/phpBB/phpbb/db/migration/tool/config.php index f93e7118c4..2a76409db5 100644 --- a/phpBB/phpbb/db/migration/tool/config.php +++ b/phpBB/phpbb/db/migration/tool/config.php @@ -150,6 +150,11 @@ class config implements \phpbb\db\migration\tool\tool_interface $arguments[0], ); break; + + case 'reverse': + // It's like double negative + $call = array_shift($arguments); + break; } if ($call) diff --git a/phpBB/phpbb/db/migration/tool/config_text.php b/phpBB/phpbb/db/migration/tool/config_text.php index bf8ac55023..21b8a8b3a0 100644 --- a/phpBB/phpbb/db/migration/tool/config_text.php +++ b/phpBB/phpbb/db/migration/tool/config_text.php @@ -115,6 +115,11 @@ class config_text implements \phpbb\db\migration\tool\tool_interface $arguments[] = ''; } break; + + case 'reverse': + // It's like double negative + $call = array_shift($arguments); + break; } if ($call) diff --git a/phpBB/phpbb/db/migration/tool/module.php b/phpBB/phpbb/db/migration/tool/module.php index 035625b095..a6baf40dc1 100644 --- a/phpBB/phpbb/db/migration/tool/module.php +++ b/phpBB/phpbb/db/migration/tool/module.php @@ -454,6 +454,11 @@ class module implements \phpbb\db\migration\tool\tool_interface case 'remove': $call = 'add'; break; + + case 'reverse': + // It's like double negative + $call = array_shift($arguments); + break; } if ($call) diff --git a/phpBB/phpbb/db/migration/tool/permission.php b/phpBB/phpbb/db/migration/tool/permission.php index ceff6d7d5a..3a1e2e344b 100644 --- a/phpBB/phpbb/db/migration/tool/permission.php +++ b/phpBB/phpbb/db/migration/tool/permission.php @@ -637,6 +637,11 @@ class permission implements \phpbb\db\migration\tool\tool_interface $arguments[0], ); break; + + case 'reverse': + // It's like double negative + $call = array_shift($arguments); + break; } if ($call) diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index 739959d7b7..3e69d9613e 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -423,19 +423,11 @@ class migrator if ($state['migration_data_done']) { - if ($state['migration_data_state'] !== 'revert_data') - { - $result = $this->process_data_step($migration->update_data(), $state['migration_data_state'], true); - - $state['migration_data_state'] = ($result === true) ? 'revert_data' : $result; - } - else - { - $result = $this->process_data_step($migration->revert_data(), '', false); + $steps = array_merge($this->helper->reverse_update_data($migration->update_data()), $migration->revert_data()); + $result = $this->process_data_step($steps, $state['migration_data_state']); - $state['migration_data_state'] = ($result === true) ? '' : $result; - $state['migration_data_done'] = ($result === true) ? false : true; - } + $state['migration_data_state'] = ($result === true) ? '' : $result; + $state['migration_data_done'] = ($result === true) ? false : true; $this->set_migration_state($name, $state); } @@ -596,6 +588,13 @@ class migrator throw new \phpbb\db\migration\exception('MIGRATION_INVALID_DATA_MISSING_STEP', $step); } + if ($reverse) + { + // We might get unexpected results when trying + // to revert this, so just avoid it + return false; + } + $condition = $parameters[0]; if (!$condition) -- cgit v1.2.1 From a277f9cf07f73e5f4563d248d1bb4eed49a7e4c9 Mon Sep 17 00:00:00 2001 From: Oliver Schramm Date: Wed, 10 Aug 2016 14:57:33 +0200 Subject: [ticket/14742] Small fixes to migrator PHPBB3-14742 --- phpBB/phpbb/db/migrator.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index 3e69d9613e..8637f71414 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -87,7 +87,7 @@ class migrator * * @var migrator_output_handler_interface */ - public $output_handler; + protected $output_handler; /** * Constructor of the database migrator @@ -333,7 +333,7 @@ class migrator $state['migration_data_done'] = ($result === true); $state['migration_end_time'] = ($result === true) ? time() : 0; - if ($state['migration_schema_done']) + if ($state['migration_data_done']) { $this->output_handler->write(array('MIGRATION_DATA_DONE', $name, $elapsed_time), migrator_output_handler_interface::VERBOSITY_NORMAL); } @@ -347,7 +347,6 @@ class migrator // Revert the schema changes $this->revert_do($name); - // Rethrow exception throw $e; } } @@ -518,7 +517,6 @@ class migrator $result = $this->run_step($reverse_step, false, !$revert); } - // rethrow the exception throw $e; } } -- cgit v1.2.1 From 2ee8bd0c4a0379961c7ec00376c216822672057f Mon Sep 17 00:00:00 2001 From: Oliver Schramm Date: Wed, 10 Aug 2016 17:57:29 +0200 Subject: [ticket/14742] Add test for reverse_update_data() PHPBB3-14742 --- tests/migrator/reverse_update_data_test.php | 56 +++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 tests/migrator/reverse_update_data_test.php diff --git a/tests/migrator/reverse_update_data_test.php b/tests/migrator/reverse_update_data_test.php new file mode 100644 index 0000000000..b85e48c64c --- /dev/null +++ b/tests/migrator/reverse_update_data_test.php @@ -0,0 +1,56 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +class reverse_update_data_test extends phpbb_test_case +{ + /** @var \phpbb\db\migration\helper */ + protected $helper; + + public function setUp() + { + parent::setUp(); + + $this->helper = new \phpbb\db\migration\helper(); + } + + public function update_data_provider() + { + return array( + array( + array( + array('config.add', array('foobar', 1)), + array('if', array( + (false === true), + array('permission.add', array('some_data')), + )), + array('config.remove', array('foobar')), + array('custom', array(array($this, 'foo_bar'))), + array('tool.method', array('test_data')), + ), + array( + array('tool.reverse', array('method', 'test_data')), + array('config.reverse', array('remove', 'foobar')), + array('config.reverse', array('add', 'foobar', 1)), + ), + ), + ); + } + + /** + * @dataProvider update_data_provider + */ + public function test_get_schema_steps($data_changes, $expected) + { + $this->assertEquals($expected, $this->helper->reverse_update_data($data_changes)); + } +} -- cgit v1.2.1