From 39ca212e17e80d14dbbd20cf5542ab37f27bd217 Mon Sep 17 00:00:00 2001 From: Nathaniel Guse Date: Sat, 2 Mar 2013 11:02:53 -0600 Subject: [PATCH 1/6] [ticket/11386] Use finder to find migration files PHPBB3-11386 --- phpBB/config/migrator.yml | 1 + phpBB/config/services.yml | 3 +- phpBB/includes/db/migrator.php | 55 +++++----------- phpBB/includes/extension/finder.php | 93 ++++++++++++++++++++++------ phpBB/includes/extension/manager.php | 11 +++- 5 files changed, 102 insertions(+), 61 deletions(-) diff --git a/phpBB/config/migrator.yml b/phpBB/config/migrator.yml index 999a2d41a3..fc534227f5 100644 --- a/phpBB/config/migrator.yml +++ b/phpBB/config/migrator.yml @@ -5,6 +5,7 @@ services: - @config - @dbal.conn - @dbal.tools + - @ext.manager - %tables.migrations% - %core.root_path% - %core.php_ext% diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index 3e4ae8d129..250e4a782b 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -116,11 +116,12 @@ services: - @service_container - @dbal.conn - @config - - @migrator - %tables.ext% - %core.root_path% - .%core.php_ext% - @cache.driver + calls: + - [set_migrator, [@migrator]] ext.finder: class: phpbb_extension_finder diff --git a/phpBB/includes/db/migrator.php b/phpBB/includes/db/migrator.php index ec0b6a87da..824bca5e70 100644 --- a/phpBB/includes/db/migrator.php +++ b/phpBB/includes/db/migrator.php @@ -31,6 +31,9 @@ class phpbb_db_migrator /** @var phpbb_db_tools */ protected $db_tools; + /** @var phpbb_extension_manager */ + protected $extension_manager; + /** @var string */ protected $table_prefix; @@ -69,11 +72,12 @@ class phpbb_db_migrator /** * Constructor of the database migrator */ - public function __construct(phpbb_config $config, phpbb_db_driver $db, phpbb_db_tools $db_tools, $migrations_table, $phpbb_root_path, $php_ext, $table_prefix, $tools) + public function __construct(phpbb_config $config, phpbb_db_driver $db, phpbb_db_tools $db_tools, phpbb_extension_manager $extension_manager, $migrations_table, $phpbb_root_path, $php_ext, $table_prefix, $tools) { $this->config = $config; $this->db = $db; $this->db_tools = $db_tools; + $this->extension_manager = $extension_manager; $this->migrations_table = $migrations_table; @@ -180,55 +184,26 @@ class phpbb_db_migrator * If FALSE, we will not check. You SHOULD check at least once * to prevent errors (if including multiple directories, check * with the last call to prevent throwing errors unnecessarily). - * @param bool $recursive Set to true to also load data files from subdirectories * @return array Array of migration names */ - public function load_migrations($path, $check_fulfillable = true, $recursive = true) + public function load_migrations($path, $check_fulfillable = true) { if (!is_dir($path)) { throw new phpbb_db_migration_exception('DIRECTORY INVALID', $path); } - $handle = opendir($path); - while (($file = readdir($handle)) !== false) + $finder = $this->extension_manager->get_finder(); + $migration_files = $finder + ->extension_directory("/") + ->find_from_paths(array('/' => $path)); + foreach ($migration_files as $migration) { - if ($file == '.' || $file == '..') + $migration_name = $migration['path'] . $migration['filename']; + + if (!in_array($migration_name, $this->migrations)) { - continue; - } - - // Recursion through subdirectories - if (is_dir($path . $file) && $recursive) - { - $this->load_migrations($path . $file . '/', $check_fulfillable, $recursive); - } - - if (strpos($file, '_') !== 0 && strrpos($file, '.' . $this->php_ext) === (strlen($file) - strlen($this->php_ext) - 1)) - { - // We try to find what class existed by comparing the classes declared before and after including the file. - $declared_classes = get_declared_classes(); - - include ($path . $file); - - $added_classes = array_diff(get_declared_classes(), $declared_classes); - - if ( - // If two classes have been added and phpbb_db_migration is one of them, we've only added one real migration - !(sizeof($added_classes) == 2 && in_array('phpbb_db_migration', $added_classes)) && - // Otherwise there should only be one class added - sizeof($added_classes) != 1 - ) - { - throw new phpbb_db_migration_exception('MIGRATION DATA FILE INVALID', $path . $file); - } - - $name = array_pop($added_classes); - - if (!in_array($name, $this->migrations)) - { - $this->migrations[] = $name; - } + $this->migrations[] = $migration_name; } } diff --git a/phpBB/includes/extension/finder.php b/phpBB/includes/extension/finder.php index fb19b98429..15e6db1bbe 100644 --- a/phpBB/includes/extension/finder.php +++ b/phpBB/includes/extension/finder.php @@ -247,15 +247,28 @@ class phpbb_extension_finder * phpBB naming rules an incorrect class name will be returned. * * @param bool $cache Whether the result should be cached + * @param bool $use_all_available Use all available instead of just all + * enabled extensions * @return array An array of found class names */ - public function get_classes($cache = true) + public function get_classes($cache = true, $use_all_available = false) { $this->query['extension_suffix'] .= $this->php_ext; $this->query['core_suffix'] .= $this->php_ext; - $files = $this->find($cache, false); + $files = $this->find($cache, false, $use_all_available); + return $this->get_classes_from_files($files); + } + + /** + * Get class names from a list of files + * + * @param array $files Array of files (from find()) + * @return array Array of class names + */ + public function get_classes_from_files($files) + { $classes = array(); foreach ($files as $file => $ext_name) { @@ -270,23 +283,27 @@ class phpbb_extension_finder * Finds all directories matching the configured options * * @param bool $cache Whether the result should be cached + * @param bool $use_all_available Use all available instead of just all + * enabled extensions * @param bool $extension_keys Whether the result should have extension name as array key * @return array An array of paths to found directories */ - public function get_directories($cache = true, $extension_keys = false) + public function get_directories($cache = true, $use_all_available = false, $extension_keys = false) { - return $this->find_with_root_path($cache, true, $extension_keys); + return $this->find_with_root_path($cache, true, $use_all_available, $extension_keys); } /** * Finds all files matching the configured options. * * @param bool $cache Whether the result should be cached + * @param bool $use_all_available Use all available instead of just all + * enabled extensions * @return array An array of paths to found files */ - public function get_files($cache = true) + public function get_files($cache = true, $use_all_available = false) { - return $this->find_with_root_path($cache, false); + return $this->find_with_root_path($cache, false, $use_all_available); } /** @@ -295,13 +312,15 @@ class phpbb_extension_finder * @param bool $cache Whether the result should be cached * @param bool $is_dir Directories will be returned when true, only files * otherwise + * @param bool $use_all_available Use all available instead of just all + * enabled extensions * @param bool $extension_keys If true, result will be associative array * with extension name as key * @return array An array of paths to found items */ - protected function find_with_root_path($cache = true, $is_dir = false, $extension_keys = false) + protected function find_with_root_path($cache = true, $is_dir = false, $use_all_available = false, $extension_keys = false) { - $items = $this->find($cache, $is_dir); + $items = $this->find($cache, $is_dir, $use_all_available); $result = array(); foreach ($items as $item => $ext_name) @@ -325,12 +344,51 @@ class phpbb_extension_finder * @param bool $cache Whether the result should be cached * @param bool $is_dir Directories will be returned when true, only files * otherwise + * @param bool $use_all_available Use all available instead of just all + * enabled extensions * @return array An array of paths to found items */ - public function find($cache = true, $is_dir = false) + public function find($cache = true, $is_dir = false, $use_all_available = false) + { + if ($use_all_available) + { + $extensions = $this->extension_manager->all_available(); + } + else + { + $extensions = $this->extension_manager->all_enabled(); + } + + if ($this->query['core_path']) + { + $extensions['/'] = $this->phpbb_root_path . $this->query['core_path']; + } + + $files = array(); + $file_list = $this->find_from_paths($extensions, $cache, $is_dir); + + foreach ($file_list as $file) + { + $files[$file['named_path']] = $file['ext_name']; + } + + return $files; + } + + /** + * Finds all file system entries matching the configured options from + * an array of paths + * + * @param array $extensions Array of extensions (name => full relative path) + * @param bool $cache Whether the result should be cached + * @param bool $is_dir Directories will be returned when true, only files + * otherwise + * @return array An array of paths to found items + */ + public function find_from_paths($extensions, $cache = true, $is_dir = false) { $this->query['is_dir'] = $is_dir; - $query = md5(serialize($this->query)); + $query = md5(serialize($this->query) . serialize($extensions)); if (!defined('DEBUG') && $cache && isset($this->cached_queries[$query])) { @@ -339,13 +397,6 @@ class phpbb_extension_finder $files = array(); - $extensions = $this->extension_manager->all_enabled(); - - if ($this->query['core_path']) - { - $extensions['/'] = $this->phpbb_root_path . $this->query['core_path']; - } - foreach ($extensions as $name => $path) { $ext_name = $name; @@ -419,7 +470,13 @@ class phpbb_extension_finder (!$prefix || substr($filename, 0, strlen($prefix)) === $prefix) && (!$directory || preg_match($directory_pattern, $relative_path))) { - $files[str_replace(DIRECTORY_SEPARATOR, '/', $location . $name . substr($relative_path, 1))] = $ext_name; + $files[] = array( + 'named_path' => str_replace(DIRECTORY_SEPARATOR, '/', $location . $name . substr($relative_path, 1)), + 'ext_name' => $ext_name, + 'path' => str_replace(array(DIRECTORY_SEPARATOR, $this->phpbb_root_path), array('/', ''), $file_info->getPath()) . '/', + 'filename' => $filename, + 'file_info' => $file_info, + ); } } } diff --git a/phpBB/includes/extension/manager.php b/phpBB/includes/extension/manager.php index 21a9ec1370..0d760681b9 100644 --- a/phpBB/includes/extension/manager.php +++ b/phpBB/includes/extension/manager.php @@ -49,13 +49,12 @@ class phpbb_extension_manager * @param phpbb_cache_driver_interface $cache A cache instance or null * @param string $cache_name The name of the cache variable, defaults to _ext */ - public function __construct(ContainerInterface $container, phpbb_db_driver $db, phpbb_config $config, phpbb_db_migrator $migrator, $extension_table, $phpbb_root_path, $php_ext = '.php', phpbb_cache_driver_interface $cache = null, $cache_name = '_ext') + public function __construct(ContainerInterface $container, phpbb_db_driver $db, phpbb_config $config, $extension_table, $phpbb_root_path, $php_ext = '.php', phpbb_cache_driver_interface $cache = null, $cache_name = '_ext') { $this->container = $container; $this->phpbb_root_path = $phpbb_root_path; $this->db = $db; $this->config = $config; - $this->migrator = $migrator; $this->cache = $cache; $this->php_ext = $php_ext; $this->extension_table = $extension_table; @@ -69,6 +68,14 @@ class phpbb_extension_manager } } + /** + * Set migrator (get around circular reference) + */ + public function set_migrator(phpbb_db_migrator $migrator) + { + $this->migrator = $migrator; + } + /** * Loads all extension information from the database * From 8415ae839cf699e043fe24ad91585dc419596ff8 Mon Sep 17 00:00:00 2001 From: Nathaniel Guse Date: Sat, 2 Mar 2013 11:37:58 -0600 Subject: [PATCH 2/6] [ticket/11386] Update tests with new constructors for ext.manager/migrator PHPBB3-11386 --- tests/dbal/migrator_test.php | 21 ++++++++++++++++++- tests/extension/manager_test.php | 17 +++++++++++++-- tests/extension/metadata_manager_test.php | 1 - .../phpbb_functional_test_case.php | 17 +++++++++++++-- 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 9460e76f37..6fc08d51f8 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -44,7 +44,26 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $tools = array( new phpbb_db_migration_tool_config($this->config), ); - $this->migrator = new phpbb_db_migrator($this->config, $this->db, $this->db_tools, 'phpbb_migrations', dirname(__FILE__) . '/../../phpBB/', 'php', 'phpbb_', $tools); + + $this->extension_manager = new phpbb_extension_manager( + new phpbb_mock_container_builder(), + $this->db, + $this->config, + 'phpbb_ext', + dirname(__FILE__) . '/../../phpBB/', + '.php', + null + ); + $this->migrator = new phpbb_db_migrator( + $this->config, + $this->db, + $this->db_tools, + $this->extension_manager, + 'phpbb_migrations', + dirname(__FILE__) . '/../../phpBB/', + 'php', + 'phpbb_', $tools + ); } public function test_update() diff --git a/tests/extension/manager_test.php b/tests/extension/manager_test.php index 9032afbd73..e9db928997 100644 --- a/tests/extension/manager_test.php +++ b/tests/extension/manager_test.php @@ -97,15 +97,28 @@ class phpbb_extension_manager_test extends phpbb_database_test_case $php_ext = 'php'; $table_prefix = 'phpbb_'; - return new phpbb_extension_manager( + $manager = new phpbb_extension_manager( new phpbb_mock_container_builder(), $db, $config, - new phpbb_db_migrator($config, $db, $db_tools, 'phpbb_migrations', $phpbb_root_path, $php_ext, $table_prefix, array()), 'phpbb_ext', dirname(__FILE__) . '/', '.' . $php_ext, ($with_cache) ? new phpbb_mock_cache() : null ); + $migrator = new phpbb_db_migrator( + $config, + $db, + $db_tools, + $manager, + 'phpbb_migrations', + $phpbb_root_path, + $php_ext, + $table_prefix, + array() + ); + $manager->set_migrator($migrator); + + return $manager; } } diff --git a/tests/extension/metadata_manager_test.php b/tests/extension/metadata_manager_test.php index cdea8d5258..7fb19b67e3 100644 --- a/tests/extension/metadata_manager_test.php +++ b/tests/extension/metadata_manager_test.php @@ -53,7 +53,6 @@ class metadata_manager_test extends phpbb_database_test_case new phpbb_mock_container_builder(), $this->db, $this->config, - new phpbb_db_migrator($this->config, $this->db, $this->db_tools, 'phpbb_migrations', $this->phpbb_root_path, $this->php_ext, $this->table_prefix, array()), 'phpbb_ext', $this->phpbb_root_path, $this->phpEx, diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php index b570b464e6..ff358033b1 100644 --- a/tests/test_framework/phpbb_functional_test_case.php +++ b/tests/test_framework/phpbb_functional_test_case.php @@ -138,16 +138,29 @@ class phpbb_functional_test_case extends phpbb_test_case $db = $this->get_db(); $db_tools = new phpbb_db_tools($db); - return new phpbb_extension_manager( + $extension_manager = new phpbb_extension_manager( new phpbb_mock_container_builder(), $db, $config, - new phpbb_db_migrator($config, $db, $db_tools, self::$config['table_prefix'] . 'migrations', $phpbb_root_path, $php_ext, self::$config['table_prefix'], array()), self::$config['table_prefix'] . 'ext', dirname(__FILE__) . '/', '.' . $php_ext, $this->get_cache_driver() ); + $migrator = new phpbb_db_migrator( + $config, + $db, + $db_tools, + $manager, + self::$config['table_prefix'] . 'migrations', + $phpbb_root_path, + $php_ext, + self::$config['table_prefix'], + array() + ); + $extension_manager->set_migrator($migrator); + + return $extension_manager; } static protected function install_board() From 1368470f7488b278cdc214745a7d4c9557d407e2 Mon Sep 17 00:00:00 2001 From: Nathaniel Guse Date: Sat, 2 Mar 2013 11:42:30 -0600 Subject: [PATCH 3/6] [ticket/11386] Forgot to get the migration classes PHPBB3-11386 --- phpBB/includes/db/migrator.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/phpBB/includes/db/migrator.php b/phpBB/includes/db/migrator.php index 824bca5e70..855e640554 100644 --- a/phpBB/includes/db/migrator.php +++ b/phpBB/includes/db/migrator.php @@ -193,17 +193,23 @@ class phpbb_db_migrator throw new phpbb_db_migration_exception('DIRECTORY INVALID', $path); } + $migrations = array(); + $finder = $this->extension_manager->get_finder(); - $migration_files = $finder + $files = $finder ->extension_directory("/") ->find_from_paths(array('/' => $path)); - foreach ($migration_files as $migration) + foreach ($files as $file) { - $migration_name = $migration['path'] . $migration['filename']; + $migrations[$file['path'] . $file['filename']] = ''; + } + $migrations = $finder->get_classes_from_files($migrations); - if (!in_array($migration_name, $this->migrations)) + foreach ($migrations as $migration) + { + if (!in_array($migration, $this->migrations)) { - $this->migrations[] = $migration_name; + $this->migrations[] = $migration; } } From 024c21f30d9873aa21d275ab85a6d662d9024089 Mon Sep 17 00:00:00 2001 From: Nathaniel Guse Date: Sat, 2 Mar 2013 11:55:28 -0600 Subject: [PATCH 4/6] [ticket/11386] Remove tests that check if finder cache is working These don't seem necessary and are much more complicated to get working now with the changes in this PR PHPBB3-11386 --- tests/extension/finder_test.php | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/extension/finder_test.php b/tests/extension/finder_test.php index 622f404786..c96b11a73c 100644 --- a/tests/extension/finder_test.php +++ b/tests/extension/finder_test.php @@ -142,6 +142,9 @@ class phpbb_extension_finder_test extends phpbb_test_case ); } + /** + * These do not work because of changes with PHPBB3-11386 + * They do not seem neccessary to me, so I am commenting them out for now public function test_get_classes_create_cache() { $cache = new phpbb_mock_cache; @@ -183,11 +186,15 @@ class phpbb_extension_finder_test extends phpbb_test_case 'is_dir' => false, ); - $finder = new phpbb_extension_finder($this->extension_manager, dirname(__FILE__) . '/', new phpbb_mock_cache(array( - '_ext_finder' => array( - md5(serialize($query)) => array('file_name' => 'extension'), - ), - ))); + $finder = new phpbb_extension_finder( + $this->extension_manager, + dirname(__FILE__) . '/', + new phpbb_mock_cache(array( + '_ext_finder' => array( + md5(serialize($query)) => array('file_name' => 'extension'), + ), + )) + ); $classes = $finder ->core_path($query['core_path']) @@ -200,4 +207,5 @@ class phpbb_extension_finder_test extends phpbb_test_case $classes ); } + */ } From a6f877c0d84ff102d3812246eae7469e191983e2 Mon Sep 17 00:00:00 2001 From: Nathaniel Guse Date: Sat, 2 Mar 2013 14:15:59 -0600 Subject: [PATCH 5/6] [ticket/11386] Fix circular reference error & serialize error PHPBB3-11386 --- phpBB/config/migrator.yml | 3 ++- phpBB/includes/db/migrator.php | 13 +++++++++++-- phpBB/includes/extension/finder.php | 1 - 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/phpBB/config/migrator.yml b/phpBB/config/migrator.yml index fc534227f5..42445ef9bf 100644 --- a/phpBB/config/migrator.yml +++ b/phpBB/config/migrator.yml @@ -5,12 +5,13 @@ services: - @config - @dbal.conn - @dbal.tools - - @ext.manager - %tables.migrations% - %core.root_path% - %core.php_ext% - %core.table_prefix% - @migrator.tool_collection + calls: + - [set_extension_manager, [@ext.manager]] migrator.tool_collection: class: phpbb_di_service_collection diff --git a/phpBB/includes/db/migrator.php b/phpBB/includes/db/migrator.php index 855e640554..de9c06948c 100644 --- a/phpBB/includes/db/migrator.php +++ b/phpBB/includes/db/migrator.php @@ -72,12 +72,11 @@ class phpbb_db_migrator /** * Constructor of the database migrator */ - public function __construct(phpbb_config $config, phpbb_db_driver $db, phpbb_db_tools $db_tools, phpbb_extension_manager $extension_manager, $migrations_table, $phpbb_root_path, $php_ext, $table_prefix, $tools) + public function __construct(phpbb_config $config, phpbb_db_driver $db, phpbb_db_tools $db_tools, $migrations_table, $phpbb_root_path, $php_ext, $table_prefix, $tools) { $this->config = $config; $this->db = $db; $this->db_tools = $db_tools; - $this->extension_manager = $extension_manager; $this->migrations_table = $migrations_table; @@ -94,6 +93,16 @@ class phpbb_db_migrator $this->load_migration_state(); } + /** + * Set Extension Manager (required) + * + * Not in constructor to prevent circular reference error + */ + public function set_extension_manager(phpbb_extension_manager $extension_manager) + { + $this->extension_manager = $extension_manager; + } + /** * Loads all migrations and their application state from the database. * diff --git a/phpBB/includes/extension/finder.php b/phpBB/includes/extension/finder.php index 15e6db1bbe..f71e32bc8d 100644 --- a/phpBB/includes/extension/finder.php +++ b/phpBB/includes/extension/finder.php @@ -475,7 +475,6 @@ class phpbb_extension_finder 'ext_name' => $ext_name, 'path' => str_replace(array(DIRECTORY_SEPARATOR, $this->phpbb_root_path), array('/', ''), $file_info->getPath()) . '/', 'filename' => $filename, - 'file_info' => $file_info, ); } } From 91be99822312d9a83ae4f6849eef864dfd47e4a1 Mon Sep 17 00:00:00 2001 From: Nathaniel Guse Date: Sat, 2 Mar 2013 15:17:51 -0600 Subject: [PATCH 6/6] [ticket/11386] Fix failing tests from constructor changes PHPBB3-11386 --- tests/dbal/migrator_test.php | 5 +++-- tests/extension/manager_test.php | 2 +- tests/test_framework/phpbb_functional_test_case.php | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 6fc08d51f8..b447a81cda 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -58,12 +58,13 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->config, $this->db, $this->db_tools, - $this->extension_manager, 'phpbb_migrations', dirname(__FILE__) . '/../../phpBB/', 'php', - 'phpbb_', $tools + 'phpbb_', + $tools ); + $this->migrator->set_extension_manager($this->extension_manager); } public function test_update() diff --git a/tests/extension/manager_test.php b/tests/extension/manager_test.php index e9db928997..3b81afc456 100644 --- a/tests/extension/manager_test.php +++ b/tests/extension/manager_test.php @@ -110,7 +110,6 @@ class phpbb_extension_manager_test extends phpbb_database_test_case $config, $db, $db_tools, - $manager, 'phpbb_migrations', $phpbb_root_path, $php_ext, @@ -118,6 +117,7 @@ class phpbb_extension_manager_test extends phpbb_database_test_case array() ); $manager->set_migrator($migrator); + $migrator->set_extension_manager($manager); return $manager; } diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php index ff358033b1..3b9629b9f8 100644 --- a/tests/test_framework/phpbb_functional_test_case.php +++ b/tests/test_framework/phpbb_functional_test_case.php @@ -151,7 +151,6 @@ class phpbb_functional_test_case extends phpbb_test_case $config, $db, $db_tools, - $manager, self::$config['table_prefix'] . 'migrations', $phpbb_root_path, $php_ext, @@ -159,6 +158,7 @@ class phpbb_functional_test_case extends phpbb_test_case array() ); $extension_manager->set_migrator($migrator); + $migrator->set_extension_manager($extension_manager); return $extension_manager; }