From 445667a62e80c42b5406c981d1116ef99a23df3b Mon Sep 17 00:00:00 2001
From: Nathan Guse <nathaniel.guse@gmail.com>
Date: Wed, 9 Jan 2013 16:31:56 -0600
Subject: [PATCH] [feature/migrations] Fix if method (and create a test for it)

PHPBB3-9737
---
 phpBB/includes/db/migrator.php | 20 ++++++++++----
 tests/dbal/migration/dummy.php | 14 +---------
 tests/dbal/migration/if.php    | 49 ++++++++++++++++++++++++++++++++++
 tests/dbal/migrator_test.php   | 35 +++++++++++++++++++-----
 4 files changed, 94 insertions(+), 24 deletions(-)
 create mode 100644 tests/dbal/migration/if.php

diff --git a/phpBB/includes/db/migrator.php b/phpBB/includes/db/migrator.php
index 5d8dc12f58..1042b767e8 100644
--- a/phpBB/includes/db/migrator.php
+++ b/phpBB/includes/db/migrator.php
@@ -364,6 +364,12 @@ class phpbb_db_migrator
 	protected function run_step($step, $last_result = false)
 	{
 		$callable_and_parameters = $this->get_callable_from_step($step, $last_result);
+
+		if ($callable_and_parameters === false)
+		{
+			return;
+		}
+
 		$callable = $callable_and_parameters[0];
 		$parameters = $callable_and_parameters[1];
 
@@ -377,7 +383,7 @@ class phpbb_db_migrator
 	* @param mixed $last_result Result to pass to the callable (only for 'custom' method)
 	* @return array Array with parameters for call_user_func_array(), 0 is the callable, 1 is parameters
 	*/
-	public function get_callable_from_step($step, $last_result = false)
+	protected function get_callable_from_step($step, $last_result = false)
 	{
 		$type = $step[0];
 		$parameters = $step[1];
@@ -406,6 +412,12 @@ class phpbb_db_migrator
 				}
 
 				$condition = $parameters[0];
+
+				if (!$condition)
+				{
+					return false;
+				}
+
 				$step = $parameters[1];
 
 				$callable_and_parameters = $this->get_callable_from_step($step);
@@ -413,10 +425,8 @@ class phpbb_db_migrator
 				$sub_parameters = $callable_and_parameters[1];
 
 				return array(
-					function ($condition) use ($callable, $sub_parameters) {
-						return call_user_func_array($callable, $sub_parameters);
-					},
-					array($condition),
+					$callable,
+					$sub_parameters,
 				);
 			break;
 			case 'custom':
diff --git a/tests/dbal/migration/dummy.php b/tests/dbal/migration/dummy.php
index 942c499bb5..e542493f9f 100644
--- a/tests/dbal/migration/dummy.php
+++ b/tests/dbal/migration/dummy.php
@@ -19,21 +19,9 @@ class phpbb_dbal_migration_dummy extends phpbb_db_migration
 		return array(
 			'add_columns' => array(
 				'phpbb_config' => array(
-					'extra_column' => array('UINT', 0),
+					'extra_column' => array('UINT', 1),
 				),
 			),
 		);
 	}
-
-	function update_data()
-	{
-		return array(
-			array('if', array(true, array('custom', array(array($this, 'set_extra_column'))))),
-		);
-	}
-
-	public function set_extra_column()
-	{
-		$this->sql_query('UPDATE phpbb_config SET extra_column = 1');
-	}
 }
diff --git a/tests/dbal/migration/if.php b/tests/dbal/migration/if.php
new file mode 100644
index 0000000000..aa9a5dab87
--- /dev/null
+++ b/tests/dbal/migration/if.php
@@ -0,0 +1,49 @@
+<?php
+/**
+*
+* @package testing
+* @copyright (c) 2011 phpBB Group
+* @license http://opensource.org/licenses/gpl-license.php GNU Public License
+*
+*/
+
+class phpbb_dbal_migration_if extends phpbb_db_migration
+{
+	function depends_on()
+	{
+		return array();
+	}
+
+	function update_schema()
+	{
+		return array();
+	}
+
+	function update_data()
+	{
+		return array(
+			array('if', array(
+				true,
+				array('custom', array(array(&$this, 'test_true'))),
+			)),
+			array('if', array(
+				false,
+				array('custom', array(array(&$this, 'test_false'))),
+			)),
+		);
+	}
+
+	function test_true()
+	{
+		global $migrator_test_if_true_failed;
+
+		$migrator_test_if_true_failed = false;
+	}
+
+	function test_false()
+	{
+		global $migrator_test_if_false_failed;
+
+		$migrator_test_if_false_failed = true;
+	}
+}
diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php
index 463cf9fcec..dd592d7b05 100644
--- a/tests/dbal/migrator_test.php
+++ b/tests/dbal/migrator_test.php
@@ -14,6 +14,7 @@ require_once dirname(__FILE__) . '/../../phpBB/includes/db/db_tools.php';
 
 require_once dirname(__FILE__) . '/migration/dummy.php';
 require_once dirname(__FILE__) . '/migration/unfulfillable.php';
+require_once dirname(__FILE__) . '/migration/if.php';
 
 class phpbb_dbal_migrator_test extends phpbb_database_test_case
 {
@@ -41,12 +42,6 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 		$this->migrator = new phpbb_db_migrator($this->config, $this->db, $this->db_tools, 'phpbb_migrations', dirname(__FILE__) . '/../../phpBB/', 'php', 'phpbb_', $tools);
 	}
 
-	public function tearDown()
-	{
-		// cleanup
-		$this->db_tools->sql_column_remove('phpbb_config', 'extra_column');
-	}
-
 	public function test_update()
 	{
 		$this->migrator->set_migrations(array('phpbb_dbal_migration_dummy'));
@@ -85,6 +80,9 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 					AND migration_end_time <= " . (time() + 1),
 			'End time set correctly'
 		);
+
+		// cleanup
+		$this->db_tools->sql_column_remove('phpbb_config', 'extra_column');
 	}
 
 	public function test_unfulfillable()
@@ -104,4 +102,29 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 			'Dummy migration was run, even though an unfulfillable migration was found.'
 		);
 	}
+
+	public function test_if()
+	{
+		$this->migrator->set_migrations(array('phpbb_dbal_migration_if'));
+
+		// Don't like this, but I'm not sure there is any other way to do this
+		global $migrator_test_if_true_failed, $migrator_test_if_false_failed;
+		$migrator_test_if_true_failed = true;
+		$migrator_test_if_false_failed = false;
+
+		while (!$this->migrator->finished())
+		{
+			$this->migrator->update();
+		}
+
+		if ($migrator_test_if_true_failed)
+		{
+			$this->fail('True test failed');
+		}
+
+		if ($migrator_test_if_false_failed)
+		{
+			$this->fail('False test failed');
+		}
+	}
 }