Merge branch 'ticket/14742' into ticket/14742-32x

phpBB/language/en/migrator.php

@@ -51,6 +51,7 @@ $lang = array_merge($lang, array(
 	'MIGRATION_NOT_INSTALLED'			=> 'The migration "%s" is not installed.',
 	'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_REVERT_DATA_DONE'		=> 'Reverted Data: %1$s; Time: %2$.2f seconds',

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

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)

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)

phpBB/phpbb/db/migration/tool/module.php

@@ -443,6 +443,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)

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)

phpBB/phpbb/db/migrator.php

@@ -89,7 +89,7 @@ class migrator
 	 *
 	 * @var migrator_output_handler_interface
 	 */
-	public $output_handler;
+	protected $output_handler;
 
 	/**
 	* Constructor of the database migrator
@@ -328,7 +328,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'])
 		{
@@ -346,7 +353,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);
 				}
@@ -360,7 +367,6 @@ class migrator
 				// Revert the schema changes
 				$this->revert_do($name);
 
-				// Rethrow exception
 				throw $e;
 			}
 		}
@@ -439,19 +445,11 @@ class migrator
 			$this->output_handler->write(array('MIGRATION_REVERT_DATA_RUNNING', $name), migrator_output_handler_interface::VERBOSITY_VERBOSE);
 			$elapsed_time = microtime(true);
 
-			if ($state['migration_data_state'] !== 'revert_data')
+			$steps = array_merge($this->helper->reverse_update_data($migration->update_data()), $migration->revert_data());
-			{
+			$result = $this->process_data_step($steps, $state['migration_data_state']);
-				$result = $this->process_data_step($migration->update_data(), $state['migration_data_state'], true);
 
-				$state['migration_data_state'] = ($result === true) ? 'revert_data' : $result;
+			$state['migration_data_state'] = ($result === true) ? '' : $result;
-			}
+			$state['migration_data_done'] = ($result === true) ? false : true;
-			else
-			{
-				$result = $this->process_data_step($migration->revert_data(), '', false);
-
-				$state['migration_data_state'] = ($result === true) ? '' : $result;
-				$state['migration_data_done'] = ($result === true) ? false : true;
-			}
 
 			$this->set_migration_state($name, $state);
 
@@ -532,8 +530,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,
@@ -556,7 +556,6 @@ class migrator
 					$this->run_step($reverse_step, false, !$revert);
 				}
 
-				// rethrow the exception
 				throw $e;
 			}
 		}
@@ -626,6 +625,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)

tests/migrator/reverse_update_data_test.php

@@ -0,0 +1,56 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @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));
+	}
+}