[ticket/14434] Refactored to check migrations when setting them

iMattPro · iMattPro · commit 27027deb9ce2 · 2016-01-28T12:17:35.000-08:00
PHPBB3-14434
diff --git a/phpBB/phpbb/console/command/db/list_command.php b/phpBB/phpbb/console/command/db/list_command.php
@@ -39,12 +39,6 @@ protected function execute(InputInterface $input, OutputInterface $output)
 
 		foreach ($this->load_migrations() as $name)
 		{
-			// Ignore non-migration files
-			if (\phpbb\db\migrator::is_migration($name) === false)
-			{
-				continue;
-			}
-
 			if ($this->migrator->migration_state($name) !== false)
 			{
 				$installed[] = $name;
diff --git a/phpBB/phpbb/console/command/db/migration_command.php b/phpBB/phpbb/console/command/db/migration_command.php
@@ -45,7 +45,7 @@ protected function load_migrations()
 
 		$this->migrator->set_migrations($migrations);
 
-		return $migrations;
+		return $this->migrator->get_migrations();
 	}
 
 	protected function finalise_update()
diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php
@@ -170,9 +170,27 @@ public function load_migration_state()
 	*/
 	public function set_migrations($class_names)
 	{
+		foreach ($class_names as $key => $class)
+		{
+			if (!self::is_migration($class))
+			{
+				unset($class_names[$key]);
+			}
+		}
+
 		$this->migrations = $class_names;
 	}
 
+	/**
+	 * Get the list of available migration class names
+	 *
+	 * @return array Array of all migrations available to be run
+	 */
+	public function get_migrations()
+	{
+		return $this->migrations;
+	}
+
 	/**
 	* Runs a single update step from the next migration to be applied.
 	*
@@ -226,7 +244,7 @@ protected function update_do()
 	*/
 	protected function try_apply($name)
 	{
-		if (!self::is_migration($name))
+		if (!class_exists($name))
 		{
 			$this->output_handler->write(array('MIGRATION_NOT_VALID', $name), migrator_output_handler_interface::VERBOSITY_DEBUG);
 			return false;
@@ -401,7 +419,7 @@ protected function revert_do($migration)
 	*/
 	protected function try_revert($name)
 	{
-		if (!self::is_migration($name))
+		if (!class_exists($name))
 		{
 			return false;
 		}
@@ -719,7 +737,7 @@ public function unfulfillable($name)
 			return false;
 		}
 
-		if (!self::is_migration($name))
+		if (!class_exists($name))
 		{
 			return $name;
 		}
diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php
@@ -73,9 +73,7 @@ public function is_enableable()
 	*/
 	public function enable_step($old_state)
 	{
-		$migrations = $this->get_migration_file_list();
-
-		$this->migrator->set_migrations($migrations);
+		$this->get_migration_file_list();
 
 		$this->migrator->update();
 
@@ -103,8 +101,6 @@ public function purge_step($old_state)
 	{
 		$migrations = $this->get_migration_file_list();
 
-		$this->migrator->set_migrations($migrations);
-
 		foreach ($migrations as $migration)
 		{
 			while ($this->migrator->migration_state($migration) !== false)
@@ -137,16 +133,9 @@ protected function get_migration_file_list()
 
 		$migrations = $this->extension_finder->get_classes_from_files($migrations);
 
-		// Unset classes that are not a valid migration
-		foreach ($migrations as $key => $migration)
-		{
-			if (\phpbb\db\migrator::is_migration($migration) === true)
-			{
-				continue;
-			}
+		$this->migrator->set_migrations($migrations);
 
-			unset($migrations[$key]);
-		}
+		$migrations = $this->migrator->get_migrations();
 
 		return $migrations;
 	}
diff --git a/phpBB/phpbb/install/module/update_database/task/update.php b/phpBB/phpbb/install/module/update_database/task/update.php
@@ -139,17 +139,8 @@ public function run()
 			->extension_directory('/migrations')
 			->get_classes();
 
-		// Unset classes that are not a valid migration
-		foreach ($migrations as $key => $migration_class)
-		{
-			if (\phpbb\db\migrator::is_migration($migration_class) === false)
-			{
-				unset($migrations[$key]);
-			}
-		}
-
 		$this->migrator->set_migrations($migrations);
-		$migration_count = count($migrations);
+		$migration_count = count($this->migrator->get_migrations());
 		$this->iohandler->set_task_count($migration_count, true);
 		$progress_count = $this->installer_config->get('database_update_count', 0);
 
diff --git a/tests/mock/migrator.php b/tests/mock/migrator.php
@@ -21,10 +21,6 @@ public function load_migration_state()
 	{
 	}
 
-	public function set_migrations($class_names)
-	{
-	}
-
 	public function update()
 	{
 	}

Original file line number	Diff line number	Diff line change
`@@ -39,12 +39,6 @@ protected function execute(InputInterface $input, OutputInterface $output)`
`39`	`39`
`40`	`40`	`foreach ($this->load_migrations() as $name)`
`41`	`41`	`{`
`42`		`- // Ignore non-migration files`
`43`		`- if (\phpbb\db\migrator::is_migration($name) === false)`
`44`		`- {`
`45`		`- continue;`
`46`		`- }`
`47`		`-`
`48`	`42`	`if ($this->migrator->migration_state($name) !== false)`
`49`	`43`	`{`
`50`	`44`	`$installed[] = $name;`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ protected function load_migrations()`
`45`	`45`
`46`	`46`	`$this->migrator->set_migrations($migrations);`
`47`	`47`
`48`		`- return $migrations;`
	`48`	`+ return $this->migrator->get_migrations();`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`protected function finalise_update()`
Original file line number	Diff line number	Diff line change
`@@ -170,9 +170,27 @@ public function load_migration_state()`
`170`	`170`	`*/`
`171`	`171`	`public function set_migrations($class_names)`
`172`	`172`	`{`
	`173`	`+ foreach ($class_names as $key => $class)`
	`174`	`+ {`
	`175`	`+ if (!self::is_migration($class))`
	`176`	`+ {`
	`177`	`+ unset($class_names[$key]);`
	`178`	`+ }`
	`179`	`+ }`
	`180`	`+`
`173`	`181`	`$this->migrations = $class_names;`
`174`	`182`	`}`
`175`	`183`
	`184`	`+ /**`
	`185`	`+ * Get the list of available migration class names`
	`186`	`+ *`
	`187`	`+ * @return array Array of all migrations available to be run`
	`188`	`+ */`
	`189`	`+ public function get_migrations()`
	`190`	`+ {`
	`191`	`+ return $this->migrations;`
	`192`	`+ }`
	`193`	`+`
`176`	`194`	`/**`
`177`	`195`	`* Runs a single update step from the next migration to be applied.`
`178`	`196`	`*`
`@@ -226,7 +244,7 @@ protected function update_do()`
`226`	`244`	`*/`
`227`	`245`	`protected function try_apply($name)`
`228`	`246`	`{`
`229`		`- if (!self::is_migration($name))`
	`247`	`+ if (!class_exists($name))`
`230`	`248`	`{`
`231`	`249`	`$this->output_handler->write(array('MIGRATION_NOT_VALID', $name), migrator_output_handler_interface::VERBOSITY_DEBUG);`
`232`	`250`	`return false;`
`@@ -401,7 +419,7 @@ protected function revert_do($migration)`
`401`	`419`	`*/`
`402`	`420`	`protected function try_revert($name)`
`403`	`421`	`{`
`404`		`- if (!self::is_migration($name))`
	`422`	`+ if (!class_exists($name))`
`405`	`423`	`{`
`406`	`424`	`return false;`
`407`	`425`	`}`
`@@ -719,7 +737,7 @@ public function unfulfillable($name)`
`719`	`737`	`return false;`
`720`	`738`	`}`
`721`	`739`
`722`		`- if (!self::is_migration($name))`
	`740`	`+ if (!class_exists($name))`
`723`	`741`	`{`
`724`	`742`	`return $name;`
`725`	`743`	`}`
Original file line number	Diff line number	Diff line change
`@@ -21,10 +21,6 @@ public function load_migration_state()`
`21`	`21`	`{`
`22`	`22`	`}`
`23`	`23`
`24`		`- public function set_migrations($class_names)`
`25`		`- {`
`26`		`- }`
`27`		`-`
`28`	`24`	`public function update()`
`29`	`25`	`{`
`30`	`26`	`}`