Merge pull request #4126 from marc1706/ticket/13733

[ticket/13733] Allow non-migration files inside migrations folder

* marc1706/ticket/13733:
  [ticket/13733] Remove validate_classes method argument
  [ticket/13733] Make sure migration classes always have same order in tests
  [ticket/13733] Properly test setting validate_classes to false/true
  [ticket/13733] Use interface for migratinos
  [ticket/13733] Update comment explaining migration class validation
  [ticket/13733] Add isInstantiable() check.
  [ticket/13733] Braces on their own lines
  [ticket/13733] Allow tests the skip class validation
  [ticket/13733] Properly handle nonexistent classes as well
  [ticket/13733] Handle nonexistent classes as well
  [ticket/13733] Only use migration classes that extension the base migration class.

phpBB/phpbb/db/migration/migration.php

@@ -20,7 +20,7 @@ namespace phpbb\db\migration;
 * in a subclass. This class provides various utility methods to simplify editing
 * a phpBB.
 */
-abstract class migration
+abstract class migration implements migration_interface
 {
 	/** @var \phpbb\config\config */
 	protected $config;
@@ -70,9 +70,7 @@ abstract class migration
 	}
 
 	/**
-	* Defines other migrations to be applied first
+	* {@inheritdoc}
-	*
-	* @return array An array of migration class names
 	*/
 	static public function depends_on()
 	{
@@ -80,14 +78,7 @@ abstract class migration
 	}
 
 	/**
-	* Allows you to check if the migration is effectively installed (entirely optional)
+	* {@inheritdoc}
-	*
-	* This is checked when a migration is installed. If true is returned, the migration will be set as
-	* installed without performing the database changes.
-	* This function is intended to help moving to migrations from a previous database updater, where some
-	* migrations may have been installed already even though they are not yet listed in the migrations table.
-	*
-	* @return bool True if this migration is installed, False if this migration is not installed (checked on install)
 	*/
 	public function effectively_installed()
 	{
@@ -95,9 +86,7 @@ abstract class migration
 	}
 
 	/**
-	* Updates the database schema by providing a set of change instructions
+	* {@inheritdoc}
-	*
-	* @return array Array of schema changes (compatible with db_tools->perform_schema_changes())
 	*/
 	public function update_schema()
 	{
@@ -105,9 +94,7 @@ abstract class migration
 	}
 
 	/**
-	* Reverts the database schema by providing a set of change instructions
+	* {@inheritdoc}
-	*
-	* @return array Array of schema changes (compatible with db_tools->perform_schema_changes())
 	*/
 	public function revert_schema()
 	{
@@ -115,9 +102,7 @@ abstract class migration
 	}
 
 	/**
-	* Updates data by returning a list of instructions to be executed
+	* {@inheritdoc}
-	*
-	* @return array Array of data update instructions
 	*/
 	public function update_data()
 	{
@@ -125,12 +110,7 @@ abstract class migration
 	}
 
 	/**
-	* Reverts data by returning a list of instructions to be executed
+	* {@inheritdoc}
-	*
-	* @return array Array of data instructions that will be performed on revert
-	* 	NOTE: calls to tools (such as config.add) are automatically reverted when
-	* 		possible, so you should not attempt to revert those, this is mostly for
-	* 		otherwise unrevertable calls (custom functions for example)
 	*/
 	public function revert_data()
 	{

phpBB/phpbb/db/migration/migration_interface.php

@@ -0,0 +1,70 @@
+<?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.
+ *
+ */
+
+namespace phpbb\db\migration;
+
+/**
+ * Base class interface for database migrations
+ */
+interface migration_interface
+{
+	/**
+	 * Defines other migrations to be applied first
+	 *
+	 * @return array An array of migration class names
+	 */
+	static public function depends_on();
+
+	/**
+	 * Allows you to check if the migration is effectively installed (entirely optional)
+	 *
+	 * This is checked when a migration is installed. If true is returned, the migration will be set as
+	 * installed without performing the database changes.
+	 * This function is intended to help moving to migrations from a previous database updater, where some
+	 * migrations may have been installed already even though they are not yet listed in the migrations table.
+	 *
+	 * @return bool True if this migration is installed, False if this migration is not installed (checked on install)
+	 */
+	public function effectively_installed();
+
+	/**
+	 * Updates the database schema by providing a set of change instructions
+	 *
+	 * @return array Array of schema changes (compatible with db_tools->perform_schema_changes())
+	 */
+	public function update_schema();
+
+	/**
+	 * Reverts the database schema by providing a set of change instructions
+	 *
+	 * @return array Array of schema changes (compatible with db_tools->perform_schema_changes())
+	 */
+	public function revert_schema();
+
+	/**
+	 * Updates data by returning a list of instructions to be executed
+	 *
+	 * @return array Array of data update instructions
+	 */
+	public function update_data();
+
+	/**
+	 * Reverts data by returning a list of instructions to be executed
+	 *
+	 * @return array Array of data instructions that will be performed on revert
+	 * 	NOTE: calls to tools (such as config.add) are automatically reverted when
+	 * 		possible, so you should not attempt to revert those, this is mostly for
+	 * 		otherwise unrevertable calls (custom functions for example)
+	 */
+	public function revert_data();
+}

phpBB/phpbb/extension/base.php

@@ -24,7 +24,7 @@ class base implements \phpbb\extension\extension_interface
 	protected $container;
 
 	/** @var \phpbb\finder */
-	protected $finder;
+	protected $extension_finder;
 
 	/** @var \phpbb\db\migrator */
 	protected $migrator;
@@ -137,6 +137,22 @@ class base implements \phpbb\extension\extension_interface
 
 		$migrations = $this->extension_finder->get_classes_from_files($migrations);
 
+		// Unset classes that do not exist or do not extend the
+		// abstract class phpbb\db\migration\migration
+		foreach ($migrations as $key => $migration)
+		{
+			if (class_exists($migration))
+			{
+				$reflector = new \ReflectionClass($migration);
+				if ($reflector->implementsInterface('\phpbb\db\migration\migration_interface') && $reflector->isInstantiable())
+				{
+					continue;
+				}
+			}
+
+			unset($migrations[$key]);
+		}
+
 		return $migrations;
 	}
 }

tests/extension/ext/vendor2/bar/migrations/bar.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace vendor2\foo\migrations;
+
+class bar
+{
+}

tests/extension/ext/vendor2/bar/migrations/foo.php

@@ -0,0 +1,54 @@
+<?php
+
+namespace vendor2\foo\migrations;
+
+class foo implements \phpbb\db\migration\migration_interface
+{
+	/**
+	 * {@inheritdoc}
+	 */
+	static public function depends_on()
+	{
+		return array();
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function effectively_installed()
+	{
+		return false;
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function update_schema()
+	{
+		return array();
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function revert_schema()
+	{
+		return array();
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function update_data()
+	{
+		return array();
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function revert_data()
+	{
+		return array();
+	}
+}

tests/extension/extension_base_test.php

@@ -11,6 +11,9 @@
 *
 */
 require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php';
+require_once dirname(__FILE__) . '/ext/vendor2/bar/migrations/bar.php';
+require_once dirname(__FILE__) . '/ext/vendor2/bar/migrations/foo.php';
+require_once dirname(__FILE__) . '/ext/vendor2/bar/migrations/migration.php';
 
 class phpbb_extension_extension_base_test extends phpbb_test_case
 {
@@ -61,9 +64,7 @@ class phpbb_extension_extension_base_test extends phpbb_test_case
 		return array(
 			array(
 				'vendor2/bar',
-				array(
+				array('\vendor2\bar\migrations\migration'),
-					'\vendor2\bar\migrations\migration',
-				),
 			),
 		);
 	}
@@ -74,6 +75,8 @@ class phpbb_extension_extension_base_test extends phpbb_test_case
 	public function test_suffix_get_classes($extension_name, $expected)
 	{
 		$extension = $this->extension_manager->get_extension($extension_name);
-		$this->assertEquals($expected, self::$reflection_method_get_migration_file_list->invoke($extension));
+		$migration_classes = self::$reflection_method_get_migration_file_list->invoke($extension);
+		sort($migration_classes);
+		$this->assertEquals($expected, $migration_classes);
 	}
 }