From 7d2a58e27100e5c776b44223e2cc6837c293db02 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Tue, 26 Jan 2016 11:45:06 -0800 Subject: [PATCH 1/8] [ticket/14434] Schema generator should ignore migration helpers PHPBB3-14434 --- phpBB/phpbb/db/migration/schema_generator.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/phpBB/phpbb/db/migration/schema_generator.php b/phpBB/phpbb/db/migration/schema_generator.php index 7003844bc4..55ab4452ed 100644 --- a/phpBB/phpbb/db/migration/schema_generator.php +++ b/phpBB/phpbb/db/migration/schema_generator.php @@ -77,8 +77,18 @@ class schema_generator $check_dependencies = true; while (!empty($migrations)) { - foreach ($migrations as $migration_class) + foreach ($migrations as $key => $migration_class) { + if (class_exists($migration_class)) + { + $reflector = new \ReflectionClass($migration_class); + if (!$reflector->implementsInterface('\phpbb\db\migration\migration_interface') || !$reflector->isInstantiable()) + { + unset($migrations[$key]); + continue; + } + } + $open_dependencies = array_diff($migration_class::depends_on(), $tree); if (empty($open_dependencies)) From 47d8aeebde6f763ec7247daf0df16dd2388b25b6 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Wed, 27 Jan 2016 10:50:22 -0800 Subject: [PATCH 2/8] [ticket/14434] Extract migration check to a reusable method PHPBB3-14434 --- phpBB/phpbb/db/migration/schema_generator.php | 12 +++-- phpBB/phpbb/db/migrator.php | 44 +++++++++++++++++-- phpBB/phpbb/extension/base.php | 14 +----- 3 files changed, 47 insertions(+), 23 deletions(-) diff --git a/phpBB/phpbb/db/migration/schema_generator.php b/phpBB/phpbb/db/migration/schema_generator.php index 55ab4452ed..dc685bb161 100644 --- a/phpBB/phpbb/db/migration/schema_generator.php +++ b/phpBB/phpbb/db/migration/schema_generator.php @@ -79,14 +79,12 @@ class schema_generator { foreach ($migrations as $key => $migration_class) { - if (class_exists($migration_class)) + // Unset classes that do not exist or do not extend the + // abstract class phpbb\db\migration\migration + if (\phpbb\db\migrator::is_migration($migration_class) === false) { - $reflector = new \ReflectionClass($migration_class); - if (!$reflector->implementsInterface('\phpbb\db\migration\migration_interface') || !$reflector->isInstantiable()) - { - unset($migrations[$key]); - continue; - } + unset($migrations[$key]); + continue; } $open_dependencies = array_diff($migration_class::depends_on(), $tree); diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index d91860949a..563958b258 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -226,7 +226,7 @@ class migrator */ protected function try_apply($name) { - if (!class_exists($name)) + if (!class_exists($name) || !self::is_migration($name)) { $this->output_handler->write(array('MIGRATION_NOT_VALID', $name), migrator_output_handler_interface::VERBOSITY_DEBUG); return false; @@ -401,7 +401,7 @@ class migrator */ protected function try_revert($name) { - if (!class_exists($name)) + if (!class_exists($name) || !self::is_migration($name)) { return false; } @@ -719,7 +719,7 @@ class migrator return false; } - if (!class_exists($name)) + if (!class_exists($name) || !self::is_migration($name)) { return $name; } @@ -857,4 +857,42 @@ class migrator )); } } + + /** + * Check if a class is a migration. + * + * @param mixed $migration An array of migration name strings, or + * a single migration name string. + * @return bool Returns true or false for a single migration. + * If an array was received, non-migrations will + * be removed from the array, and false is returned. + */ + static public function is_migration(&$migration) + { + if (is_array($migration)) + { + foreach ($migration as $key => $name) + { + if (self::is_migration($name)) + { + continue; + } + + unset($migration[$key]); + } + } + else if (class_exists($migration)) + { + // Migration classes should extend the abstract class + // phpbb\db\migration\migration which implements the + // migration_interface and be instantiable. + $reflector = new \ReflectionClass($migration); + if ($reflector->implementsInterface('\phpbb\db\migration\migration_interface') && $reflector->isInstantiable()) + { + return true; + } + } + + return false; + } } diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index b647242b98..b05c1af8d4 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -139,19 +139,7 @@ class base implements \phpbb\extension\extension_interface // 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]); - } + \phpbb\db\migrator::is_migration($migrations); return $migrations; } From 3bd8a2ba196f240c5b982271af1ee8b2a2f8332b Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Wed, 27 Jan 2016 11:46:04 -0800 Subject: [PATCH 3/8] [ticket/14434] Remove recursion to simplify is_migration method PHPBB3-14434 --- phpBB/phpbb/db/migration/schema_generator.php | 3 +-- phpBB/phpbb/db/migrator.php | 27 +++++-------------- phpBB/phpbb/extension/base.php | 13 ++++++--- 3 files changed, 17 insertions(+), 26 deletions(-) diff --git a/phpBB/phpbb/db/migration/schema_generator.php b/phpBB/phpbb/db/migration/schema_generator.php index dc685bb161..c579e25824 100644 --- a/phpBB/phpbb/db/migration/schema_generator.php +++ b/phpBB/phpbb/db/migration/schema_generator.php @@ -79,8 +79,7 @@ class schema_generator { foreach ($migrations as $key => $migration_class) { - // Unset classes that do not exist or do not extend the - // abstract class phpbb\db\migration\migration + // Unset classes that are not a valid migration if (\phpbb\db\migrator::is_migration($migration_class) === false) { unset($migrations[$key]); diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index 563958b258..2f280ec5a5 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -861,31 +861,16 @@ class migrator /** * Check if a class is a migration. * - * @param mixed $migration An array of migration name strings, or - * a single migration name string. - * @return bool Returns true or false for a single migration. - * If an array was received, non-migrations will - * be removed from the array, and false is returned. + * @param string $migration A migration class name + * @return bool Return true if class is a migration, false otherwise */ - static public function is_migration(&$migration) + static public function is_migration($migration) { - if (is_array($migration)) - { - foreach ($migration as $key => $name) - { - if (self::is_migration($name)) - { - continue; - } - - unset($migration[$key]); - } - } - else if (class_exists($migration)) + if (class_exists($migration)) { // Migration classes should extend the abstract class - // phpbb\db\migration\migration which implements the - // migration_interface and be instantiable. + // phpbb\db\migration\migration (which implements the + // migration_interface) and be instantiable. $reflector = new \ReflectionClass($migration); if ($reflector->implementsInterface('\phpbb\db\migration\migration_interface') && $reflector->isInstantiable()) { diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index b05c1af8d4..abdb10df88 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -137,9 +137,16 @@ 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 - \phpbb\db\migrator::is_migration($migrations); + // Unset classes that are not a valid migration + foreach ($migrations as $key => $migration) + { + if (\phpbb\db\migrator::is_migration($migration) === true) + { + continue; + } + + unset($migrations[$key]); + } return $migrations; } From ae7aa5dc579f65ee2206e6ed7736d2b320bdaad1 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Wed, 27 Jan 2016 11:48:24 -0800 Subject: [PATCH 4/8] [ticket/14434] Fix whitespace mistakes PHPBB3-14434 --- phpBB/phpbb/extension/base.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/phpBB/phpbb/extension/base.php b/phpBB/phpbb/extension/base.php index abdb10df88..a7531350f7 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -143,10 +143,10 @@ class base implements \phpbb\extension\extension_interface if (\phpbb\db\migrator::is_migration($migration) === true) { continue; - } + } - unset($migrations[$key]); - } + unset($migrations[$key]); + } return $migrations; } From 996441a1da472b587d367994cb6b6dcf711b027c Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Wed, 27 Jan 2016 12:24:18 -0800 Subject: [PATCH 5/8] [ticket/14434] Remove redundant conditional PHPBB3-14434 --- phpBB/phpbb/db/migrator.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index 2f280ec5a5..8f3b860b93 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -226,7 +226,7 @@ class migrator */ protected function try_apply($name) { - if (!class_exists($name) || !self::is_migration($name)) + if (!self::is_migration($name)) { $this->output_handler->write(array('MIGRATION_NOT_VALID', $name), migrator_output_handler_interface::VERBOSITY_DEBUG); return false; @@ -401,7 +401,7 @@ class migrator */ protected function try_revert($name) { - if (!class_exists($name) || !self::is_migration($name)) + if (!self::is_migration($name)) { return false; } @@ -719,7 +719,7 @@ class migrator return false; } - if (!class_exists($name) || !self::is_migration($name)) + if (!self::is_migration($name)) { return $name; } From fc72862ca4bb5c9f9b23867173543efb899de4db Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Wed, 27 Jan 2016 12:47:31 -0800 Subject: [PATCH 6/8] [ticket/14434] Do not include non-migrations in CLI list PHPBB3-14434 --- phpBB/phpbb/console/command/db/list_command.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/phpBB/phpbb/console/command/db/list_command.php b/phpBB/phpbb/console/command/db/list_command.php index 708107b592..dfec5c7da2 100644 --- a/phpBB/phpbb/console/command/db/list_command.php +++ b/phpBB/phpbb/console/command/db/list_command.php @@ -39,6 +39,12 @@ class list_command extends \phpbb\console\command\db\migration_command 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; From fb1acb0ef463bc38421248497e7f0b7b271600f7 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 28 Jan 2016 07:04:55 -0800 Subject: [PATCH 7/8] [ticket/14434] Check migrations in the database updater task PHPBB3-14434 --- .../phpbb/install/module/update_database/task/update.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/phpBB/phpbb/install/module/update_database/task/update.php b/phpBB/phpbb/install/module/update_database/task/update.php index 84ec6f73f5..eb9bdc8138 100644 --- a/phpBB/phpbb/install/module/update_database/task/update.php +++ b/phpBB/phpbb/install/module/update_database/task/update.php @@ -139,6 +139,15 @@ class update extends task_base ->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); $this->iohandler->set_task_count($migration_count, true); From 27027deb9ce2076f64dbfdecba494efe1aa523dc Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 28 Jan 2016 11:22:30 -0800 Subject: [PATCH 8/8] [ticket/14434] Refactored to check migrations when setting them PHPBB3-14434 --- .../phpbb/console/command/db/list_command.php | 6 ----- .../console/command/db/migration_command.php | 2 +- phpBB/phpbb/db/migrator.php | 24 ++++++++++++++++--- phpBB/phpbb/extension/base.php | 17 +++---------- .../module/update_database/task/update.php | 11 +-------- tests/mock/migrator.php | 4 ---- 6 files changed, 26 insertions(+), 38 deletions(-) diff --git a/phpBB/phpbb/console/command/db/list_command.php b/phpBB/phpbb/console/command/db/list_command.php index dfec5c7da2..708107b592 100644 --- a/phpBB/phpbb/console/command/db/list_command.php +++ b/phpBB/phpbb/console/command/db/list_command.php @@ -39,12 +39,6 @@ class list_command extends \phpbb\console\command\db\migration_command 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 index d44ef8c5cb..b951560588 100644 --- a/phpBB/phpbb/console/command/db/migration_command.php +++ b/phpBB/phpbb/console/command/db/migration_command.php @@ -45,7 +45,7 @@ abstract class migration_command extends \phpbb\console\command\command $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 index 8f3b860b93..a1e93942cd 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -170,9 +170,27 @@ class migrator */ 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 @@ class migrator */ 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 @@ class migrator */ protected function try_revert($name) { - if (!self::is_migration($name)) + if (!class_exists($name)) { return false; } @@ -719,7 +737,7 @@ class migrator 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 index a7531350f7..c7778cfed1 100644 --- a/phpBB/phpbb/extension/base.php +++ b/phpBB/phpbb/extension/base.php @@ -73,9 +73,7 @@ class base implements \phpbb\extension\extension_interface */ 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 @@ class base implements \phpbb\extension\extension_interface { $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 @@ class base implements \phpbb\extension\extension_interface $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 index eb9bdc8138..4b2baf2c23 100644 --- a/phpBB/phpbb/install/module/update_database/task/update.php +++ b/phpBB/phpbb/install/module/update_database/task/update.php @@ -139,17 +139,8 @@ class update extends task_base ->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 index 293f115335..4d1aca0a0a 100644 --- a/tests/mock/migrator.php +++ b/tests/mock/migrator.php @@ -21,10 +21,6 @@ class phpbb_mock_migrator extends \phpbb\db\migrator { } - public function set_migrations($class_names) - { - } - public function update() { }