From 715d365a5e776207e1dddac7e5ccc50aad5621f1 Mon Sep 17 00:00:00 2001 From: David King Date: Sat, 23 May 2015 17:06:25 -0400 Subject: [PATCH 01/11] [ticket/13733] Only use migration classes that extension the base migration class. PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index 5bb530bad4..4bf19b37ed 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -137,6 +137,14 @@ class base implements \phpbb\extension\extension_interface $migrations = $this->extension_finder->get_classes_from_files($migrations); + foreach ($migrations as $key => $migration) + { + $reflector = new \ReflectionClass($migration); + if (!$reflector->isSubclassOf('\phpbb\db\migration\migration')) { + unset($migrations[$key]); + } + } + return $migrations; } } From 9e6f9c8a64d89f48d531c5b24d535025dd04f956 Mon Sep 17 00:00:00 2001 From: David King Date: Sat, 23 May 2015 19:57:32 -0400 Subject: [PATCH 02/11] [ticket/13733] Handle nonexistent classes as well PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index 4bf19b37ed..8b4d747eaf 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -139,8 +139,9 @@ class base implements \phpbb\extension\extension_interface foreach ($migrations as $key => $migration) { - $reflector = new \ReflectionClass($migration); - if (!$reflector->isSubclassOf('\phpbb\db\migration\migration')) { + // If the class doesn't exist OR the class does not extend the migration class + // we need to skip it. + if (!class_exists($migration) || ($reflector = new \ReflectionClass($migration) && !$reflector->isSubclassOf('\phpbb\db\migration\migration'))) { unset($migrations[$key]); } } From 4ecc13af83718e26e02afc9b8516a506ca5a26e1 Mon Sep 17 00:00:00 2001 From: David King Date: Sat, 23 May 2015 20:13:23 -0400 Subject: [PATCH 03/11] [ticket/13733] Properly handle nonexistent classes as well PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index 8b4d747eaf..8e717e1beb 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -139,11 +139,21 @@ class base implements \phpbb\extension\extension_interface foreach ($migrations as $key => $migration) { - // If the class doesn't exist OR the class does not extend the migration class - // we need to skip it. - if (!class_exists($migration) || ($reflector = new \ReflectionClass($migration) && !$reflector->isSubclassOf('\phpbb\db\migration\migration'))) { - unset($migrations[$key]); + // If the class exists and is a subclass of the + // \phpbb\db\migration\migration abstract class + // we skip it. + + // Otherwise, i.e. if it doesn't exist or it is + // not an extend the abstract class, we unset it + if (class_exists($migration)) { + $reflector = new \ReflectionClass($migration); + if ($reflector->isSubclassOf('\phpbb\db\migration\migration')) { + continue; + } + } + + unset($migrations[$key]); } return $migrations; From 65316cffafead1b0529dca50f4c110489615438a Mon Sep 17 00:00:00 2001 From: David King Date: Sat, 23 May 2015 21:13:30 -0400 Subject: [PATCH 04/11] [ticket/13733] Allow tests the skip class validation PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 31 ++++++++++++++----------- tests/extension/extension_base_test.php | 2 +- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index 8e717e1beb..ed190f6aa5 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -121,9 +121,11 @@ class base implements \phpbb\extension\extension_interface /** * Get the list of migration files from this extension * + * @var bool $validate_classes Whether or not to check that the migration + * class exists and extends the base migration class. * @return array */ - protected function get_migration_file_list() + protected function get_migration_file_list($validate_classes = true) { if ($this->migrations !== false) { @@ -137,23 +139,26 @@ class base implements \phpbb\extension\extension_interface $migrations = $this->extension_finder->get_classes_from_files($migrations); - foreach ($migrations as $key => $migration) + if ($validate_classes) { - // If the class exists and is a subclass of the - // \phpbb\db\migration\migration abstract class - // we skip it. + foreach ($migrations as $key => $migration) + { + // If the class exists and is a subclass of the + // \phpbb\db\migration\migration abstract class + // we skip it. + + // Otherwise, i.e. if it doesn't exist or it is + // not an extend the abstract class, we unset it + if (class_exists($migration)) { + $reflector = new \ReflectionClass($migration); + if ($reflector->isSubclassOf('\phpbb\db\migration\migration')) { + continue; + } - // Otherwise, i.e. if it doesn't exist or it is - // not an extend the abstract class, we unset it - if (class_exists($migration)) { - $reflector = new \ReflectionClass($migration); - if ($reflector->isSubclassOf('\phpbb\db\migration\migration')) { - continue; } + unset($migrations[$key]); } - - unset($migrations[$key]); } return $migrations; diff --git a/tests/extension/extension_base_test.php b/tests/extension/extension_base_test.php index eee38186db..898c11d902 100644 --- a/tests/extension/extension_base_test.php +++ b/tests/extension/extension_base_test.php @@ -74,6 +74,6 @@ 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)); + $this->assertEquals($expected, self::$reflection_method_get_migration_file_list->invoke($extension, false)); } } From c485540f537b61a1e95f72a35dbca40f0f9b1c24 Mon Sep 17 00:00:00 2001 From: David King Date: Sat, 23 May 2015 21:38:59 -0400 Subject: [PATCH 05/11] [ticket/13733] Braces on their own lines PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index ed190f6aa5..5ce6983edf 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -149,9 +149,11 @@ class base implements \phpbb\extension\extension_interface // Otherwise, i.e. if it doesn't exist or it is // not an extend the abstract class, we unset it - if (class_exists($migration)) { + if (class_exists($migration)) + { $reflector = new \ReflectionClass($migration); - if ($reflector->isSubclassOf('\phpbb\db\migration\migration')) { + if ($reflector->isSubclassOf('\phpbb\db\migration\migration')) + { continue; } From 9dc1729e379691c97b319a12912dc48ad779a286 Mon Sep 17 00:00:00 2001 From: David King Date: Sun, 24 May 2015 01:32:01 -0400 Subject: [PATCH 06/11] [ticket/13733] Add isInstantiable() check. PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index 5ce6983edf..40bd349c4d 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -152,7 +152,7 @@ class base implements \phpbb\extension\extension_interface if (class_exists($migration)) { $reflector = new \ReflectionClass($migration); - if ($reflector->isSubclassOf('\phpbb\db\migration\migration')) + if ($reflector->isSubclassOf('\phpbb\db\migration\migration') && $reflector->isInstantiable()) { continue; } From b90852485335455a3eda0e7193b40b1364156b27 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 15 Jan 2016 18:34:12 +0100 Subject: [PATCH 07/11] [ticket/13733] Update comment explaining migration class validation PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index 40bd349c4d..7eead6df1d 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -141,14 +141,10 @@ class base implements \phpbb\extension\extension_interface if ($validate_classes) { + // Unset classes that do not exist or do not extend the + // abstract class phpbb\db\migration\migration foreach ($migrations as $key => $migration) { - // If the class exists and is a subclass of the - // \phpbb\db\migration\migration abstract class - // we skip it. - - // Otherwise, i.e. if it doesn't exist or it is - // not an extend the abstract class, we unset it if (class_exists($migration)) { $reflector = new \ReflectionClass($migration); From e954b0b82b9fe873211bdd8885aefb78284f0893 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 18 Jan 2016 14:54:54 +0100 Subject: [PATCH 08/11] [ticket/13733] Use interface for migratinos PHPBB3-13733 --- phpBB/phpbb/db/migration/migration.php | 34 ++------- .../db/migration/migration_interface.php | 70 +++++++++++++++++++ phpBB/phpbb/extension/base.php | 4 +- 3 files changed, 79 insertions(+), 29 deletions(-) create mode 100644 phpBB/phpbb/db/migration/migration_interface.php diff --git a/phpBB/phpbb/db/migration/migration.php b/phpBB/phpbb/db/migration/migration.php index 2304c8e44c..4e218344f4 100644 --- a/phpBB/phpbb/db/migration/migration.php +++ b/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 - * - * @return array An array of migration class names + * {@inheritdoc} */ static public function depends_on() { @@ -80,14 +78,7 @@ abstract class migration } /** - * 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) + * {@inheritdoc} */ public function effectively_installed() { @@ -95,9 +86,7 @@ abstract class migration } /** - * Updates the database schema by providing a set of change instructions - * - * @return array Array of schema changes (compatible with db_tools->perform_schema_changes()) + * {@inheritdoc} */ public function update_schema() { @@ -105,9 +94,7 @@ abstract class migration } /** - * Reverts the database schema by providing a set of change instructions - * - * @return array Array of schema changes (compatible with db_tools->perform_schema_changes()) + * {@inheritdoc} */ public function revert_schema() { @@ -115,9 +102,7 @@ abstract class migration } /** - * Updates data by returning a list of instructions to be executed - * - * @return array Array of data update instructions + * {@inheritdoc} */ public function update_data() { @@ -125,12 +110,7 @@ abstract class migration } /** - * 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) + * {@inheritdoc} */ public function revert_data() { diff --git a/phpBB/phpbb/db/migration/migration_interface.php b/phpBB/phpbb/db/migration/migration_interface.php new file mode 100644 index 0000000000..2aba5ec608 --- /dev/null +++ b/phpBB/phpbb/db/migration/migration_interface.php @@ -0,0 +1,70 @@ + + * @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(); +} diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index 7eead6df1d..d2c13e8270 100644 --- a/phpBB/phpbb/extension/base.php +++ b/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; @@ -148,7 +148,7 @@ class base implements \phpbb\extension\extension_interface if (class_exists($migration)) { $reflector = new \ReflectionClass($migration); - if ($reflector->isSubclassOf('\phpbb\db\migration\migration') && $reflector->isInstantiable()) + if ($reflector->implementsInterface('\phpbb\db\migration\migration_interface') && $reflector->isInstantiable()) { continue; } From 0defce65c8ade296523cd9f5788c2abf43c73513 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 19 Jan 2016 23:32:07 +0100 Subject: [PATCH 09/11] [ticket/13733] Properly test setting validate_classes to false/true PHPBB3-13733 --- .../ext/vendor2/bar/migrations/bar.php | 7 +++ .../ext/vendor2/bar/migrations/foo.php | 54 +++++++++++++++++++ tests/extension/extension_base_test.php | 17 +++++- 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 tests/extension/ext/vendor2/bar/migrations/bar.php create mode 100644 tests/extension/ext/vendor2/bar/migrations/foo.php diff --git a/tests/extension/ext/vendor2/bar/migrations/bar.php b/tests/extension/ext/vendor2/bar/migrations/bar.php new file mode 100644 index 0000000000..ea5ddb6b8b --- /dev/null +++ b/tests/extension/ext/vendor2/bar/migrations/bar.php @@ -0,0 +1,7 @@ + '\vendor2\bar\migrations\migration', + ), + ), ); } /** * @dataProvider data_test_suffix_get_classes */ - public function test_suffix_get_classes($extension_name, $expected) + public function test_suffix_get_classes($extension_name, $validate_classes, $expected) { $extension = $this->extension_manager->get_extension($extension_name); - $this->assertEquals($expected, self::$reflection_method_get_migration_file_list->invoke($extension, false)); + $this->assertEquals($expected, self::$reflection_method_get_migration_file_list->invoke($extension, $validate_classes)); } } From a60935b99db712f8eec7b9ef3b9a00ac0d9a3d51 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 20 Jan 2016 11:16:06 +0100 Subject: [PATCH 10/11] [ticket/13733] Make sure migration classes always have same order in tests PHPBB3-13733 --- tests/extension/extension_base_test.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/extension/extension_base_test.php b/tests/extension/extension_base_test.php index 1e74efc1fa..71b03a40ac 100644 --- a/tests/extension/extension_base_test.php +++ b/tests/extension/extension_base_test.php @@ -74,9 +74,7 @@ class phpbb_extension_extension_base_test extends phpbb_test_case array( 'vendor2/bar', true, - array( - 2 => '\vendor2\bar\migrations\migration', - ), + array('\vendor2\bar\migrations\migration'), ), ); } @@ -87,6 +85,8 @@ class phpbb_extension_extension_base_test extends phpbb_test_case public function test_suffix_get_classes($extension_name, $validate_classes, $expected) { $extension = $this->extension_manager->get_extension($extension_name); - $this->assertEquals($expected, self::$reflection_method_get_migration_file_list->invoke($extension, $validate_classes)); + $migration_classes = self::$reflection_method_get_migration_file_list->invoke($extension, $validate_classes); + sort($migration_classes); + $this->assertEquals($expected, $migration_classes); } } From fac4672f3f2def54fb65e325e77dea14cbc4aa6a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 24 Jan 2016 22:39:37 +0100 Subject: [PATCH 11/11] [ticket/13733] Remove validate_classes method argument PHPBB3-13733 --- phpBB/phpbb/extension/base.php | 26 ++++++++++--------------- tests/extension/extension_base_test.php | 14 ++----------- 2 files changed, 12 insertions(+), 28 deletions(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index d2c13e8270..b647242b98 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -121,11 +121,9 @@ class base implements \phpbb\extension\extension_interface /** * Get the list of migration files from this extension * - * @var bool $validate_classes Whether or not to check that the migration - * class exists and extends the base migration class. * @return array */ - protected function get_migration_file_list($validate_classes = true) + protected function get_migration_file_list() { if ($this->migrations !== false) { @@ -139,24 +137,20 @@ class base implements \phpbb\extension\extension_interface $migrations = $this->extension_finder->get_classes_from_files($migrations); - if ($validate_classes) + // Unset classes that do not exist or do not extend the + // abstract class phpbb\db\migration\migration + foreach ($migrations as $key => $migration) { - // 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)) { - if (class_exists($migration)) + $reflector = new \ReflectionClass($migration); + if ($reflector->implementsInterface('\phpbb\db\migration\migration_interface') && $reflector->isInstantiable()) { - $reflector = new \ReflectionClass($migration); - if ($reflector->implementsInterface('\phpbb\db\migration\migration_interface') && $reflector->isInstantiable()) - { - continue; - } - + continue; } - - unset($migrations[$key]); } + + unset($migrations[$key]); } return $migrations; diff --git a/tests/extension/extension_base_test.php b/tests/extension/extension_base_test.php index 71b03a40ac..775a23e198 100644 --- a/tests/extension/extension_base_test.php +++ b/tests/extension/extension_base_test.php @@ -64,16 +64,6 @@ class phpbb_extension_extension_base_test extends phpbb_test_case return array( array( 'vendor2/bar', - false, - array( - '\vendor2\bar\migrations\bar', - '\vendor2\bar\migrations\foo', - '\vendor2\bar\migrations\migration', - ), - ), - array( - 'vendor2/bar', - true, array('\vendor2\bar\migrations\migration'), ), ); @@ -82,10 +72,10 @@ class phpbb_extension_extension_base_test extends phpbb_test_case /** * @dataProvider data_test_suffix_get_classes */ - public function test_suffix_get_classes($extension_name, $validate_classes, $expected) + public function test_suffix_get_classes($extension_name, $expected) { $extension = $this->extension_manager->get_extension($extension_name); - $migration_classes = self::$reflection_method_get_migration_file_list->invoke($extension, $validate_classes); + $migration_classes = self::$reflection_method_get_migration_file_list->invoke($extension); sort($migration_classes); $this->assertEquals($expected, $migration_classes); }