From 8d1f8af7c65d0a7a4645dad763c4e48a92d0ef82 Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 15 Jun 2025 13:03:55 +0700 Subject: [PATCH 01/22] [ticket/17525] Correctly handle Doctrine DB tools exceptions, enrich error info PHPBB-17525 --- phpBB/includes/acp/acp_extensions.php | 6 ++++ phpBB/includes/functions.php | 2 +- phpBB/install/startup.php | 2 +- phpBB/phpbb/db/tools/doctrine.php | 40 +++++++++++---------------- phpBB/phpbb/install/installer.php | 4 ++- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/phpBB/includes/acp/acp_extensions.php b/phpBB/includes/acp/acp_extensions.php index e08c2e341b..626f1b09c1 100644 --- a/phpBB/includes/acp/acp_extensions.php +++ b/phpBB/includes/acp/acp_extensions.php @@ -276,6 +276,12 @@ class acp_extensions { $this->template->assign_var('MIGRATOR_ERROR', $e->getLocalisedMessage($this->user)); } + catch (\Exception $e) + { + $stack_trace = phpbb_filter_root_path(str_replace("\n", '
', $e->getTraceAsString())); + $message = $e->getMessage(); + $this->template->assign_var('MIGRATOR_ERROR', '' . $message . '

' . $stack_trace); + } $this->tpl_name = 'acp_ext_enable'; diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index f725d5f996..72241b52f0 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2716,7 +2716,7 @@ function get_backtrace() // Only show function arguments for include etc. // Other parameters may contain sensible information $argument = ''; - if (!empty($trace['args'][0]) && in_array($trace['function'], array('include', 'require', 'include_once', 'require_once'))) + if (!empty($trace['args'][0]) && in_array($trace['function'], array('include', 'require', 'include_once', 'require_once', 'try_apply'))) { $argument = htmlspecialchars(phpbb_filter_root_path($trace['args'][0]), ENT_COMPAT); } diff --git a/phpBB/install/startup.php b/phpBB/install/startup.php index b8ceb37342..0cff634795 100644 --- a/phpBB/install/startup.php +++ b/phpBB/install/startup.php @@ -107,7 +107,7 @@ function installer_msg_handler($errno, $msg_text, $errfile, $errline): bool { /** @var \phpbb\install\helper\iohandler\iohandler_interface $iohandler */ $iohandler = $phpbb_installer_container->get('installer.helper.iohandler'); - $iohandler->add_error_message($msg); + $iohandler->add_error_message($msg, get_backtrace()); $iohandler->send_response(true); exit(); } diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index aaf44aed68..65fd6b39aa 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -458,34 +458,26 @@ class doctrine implements tools_interface */ protected function alter_schema(callable $callback) { - try + $current_schema = $this->get_schema(); + $new_schema = clone $current_schema; + call_user_func($callback, $new_schema); + + $comparator = new comparator(); + $schemaDiff = $comparator->compareSchemas($current_schema, $new_schema); + $queries = $schemaDiff->toSql($this->get_schema_manager()->getDatabasePlatform()); + + if ($this->return_statements) { - $current_schema = $this->get_schema(); - $new_schema = clone $current_schema; - call_user_func($callback, $new_schema); - - $comparator = new comparator(); - $schemaDiff = $comparator->compareSchemas($current_schema, $new_schema); - $queries = $schemaDiff->toSql($this->get_schema_manager()->getDatabasePlatform()); - - if ($this->return_statements) - { - return $queries; - } - - foreach ($queries as $query) - { - // executeQuery() must be used here because $query might return a result set, for instance REPAIR does - $this->connection->executeQuery($query); - } - - return true; + return $queries; } - catch (Exception $e) + + foreach ($queries as $query) { - // @todo: check if it makes sense to properly handle the exception - return [$e->getMessage()]; + // executeQuery() must be used here because $query might return a result set, for instance REPAIR does + $this->connection->executeQuery($query); } + + return true; } /** diff --git a/phpBB/phpbb/install/installer.php b/phpBB/phpbb/install/installer.php index 23ab5df919..3664b39f2d 100644 --- a/phpBB/phpbb/install/installer.php +++ b/phpBB/phpbb/install/installer.php @@ -280,7 +280,9 @@ class installer } catch (\Exception $e) { - $this->iohandler->add_error_message($e->getMessage()); + $stack_trace = phpbb_filter_root_path(str_replace("\n", '
', $e->getTraceAsString())); + $message = $e->getMessage(); + $this->iohandler->add_error_message($message, $stack_trace); $this->iohandler->send_response(true); $fail_cleanup = true; } From 0e0214a71da29a33e83de466245166ff4942fd27 Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 15 Jun 2025 15:51:59 +0700 Subject: [PATCH 02/22] [ticket/17525] Avoid index name duplication (auth_role_id) phpbb_acl_groups and phpbb_acl_users have indexes with the same names (auth_role_id) which can cause issues on SQLite/Postgres PHPBB-17525 --- .../data/v400/rename_auth_role_id_index.php | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php diff --git a/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php b/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php new file mode 100644 index 0000000000..2b716583be --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php @@ -0,0 +1,58 @@ + + * @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\data\v400; + +use phpbb\db\migration\migration; + +class rename_auth_role_id_index extends migration +{ + public static function depends_on() + { + return [ + '\phpbb\db\migration\data\v400\dev', + ]; + } + + public function update_schema() + { + return [ + 'drop_keys' => [ + $this->table_prefix . 'acl_users' => [ + 'auth_role_id', + ], + ], + 'add_index' => [ + $this->table_prefix . 'acl_users' => [ + 'usr_auth_role_id' => ['auth_role_id'], + ], + ], + ]; + } + + public function revert_schema() + { + return [ + 'drop_keys' => [ + $this->table_prefix . 'acl_users' => [ + 'usr_auth_role_id', + ], + ], + 'add_index' => [ + $this->table_prefix . 'acl_users' => [ + 'auth_role_id' => ['auth_role_id'], + ], + ], + ]; + } +} From 7b0b95250c56171bca3a7d490b81a2f265a0e36b Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 17 Jun 2025 00:46:17 +0700 Subject: [PATCH 03/22] [ticket/17525] Avoid more index name duplication Many more tables have indexes with the same names which can cause issues on SQLite/Postgres. Also, add an option for migrations to rename indexes. PHPBB-17525 --- .../data/v400/rename_auth_role_id_index.php | 90 +++++++++++++++---- phpBB/phpbb/db/migration/helper.php | 1 + phpBB/phpbb/db/migration/schema_generator.php | 21 ++++- phpBB/phpbb/db/tools/doctrine.php | 39 ++++++++ phpBB/phpbb/db/tools/tools_interface.php | 11 +++ 5 files changed, 143 insertions(+), 19 deletions(-) diff --git a/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php b/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php index 2b716583be..9a3fd790f7 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php @@ -27,14 +27,74 @@ class rename_auth_role_id_index extends migration public function update_schema() { return [ - 'drop_keys' => [ - $this->table_prefix . 'acl_users' => [ - 'auth_role_id', + 'rename_index' => [ + $this->table_prefix . 'acl_groups' => [ + 'auth_role_id' => 'aclgrps_auth_role_id', + 'group_id' => 'aclgrps_group_id', ], - ], - 'add_index' => [ $this->table_prefix . 'acl_users' => [ - 'usr_auth_role_id' => ['auth_role_id'], + 'auth_role_id' => 'aclusrs_auth_role_id', + 'user_id' => 'aclusrs_user_id', + ], + $this->table_prefix . 'attachments' => [ + 'poster_id' => 'attchmnts_poster_id', + 'topic_id' => 'attchmnts_topic_id', + ], + $this->table_prefix . 'forums_watch' => [ + 'forum_id' => 'frmswtch_forum_id', + 'user_id' => 'frmswtch_user_id', + ], + $this->table_prefix . 'log' => [ + 'forum_id' => 'log_forum_id', + 'topic_id' => 'log_topic_id', + 'user_id' => 'log_user_id', + ], + $this->table_prefix . 'login_attempts' => [ + 'user_id' => 'lgnatmpts_user_id', + ], + $this->table_prefix . 'moderator_cache' => [ + 'forum_id' => 'mdrtrcch_forum_id', + ], + $this->table_prefix . 'oauth_states' => [ + 'user_id' => 'oauthsts_user_id', + ], + $this->table_prefix . 'oauth_tokens' => [ + 'user_id' => 'oauthtkns_user_id', + ], + $this->table_prefix . 'poll_options' => [ + 'topic_id' => 'pllopts_topic_id', + ], + $this->table_prefix . 'poll_votes' => [ + 'topic_id' => 'pllvts_topic_id', + ], + $this->table_prefix . 'posts' => [ + 'forum_id' => 'psts_forum_id', + 'poster_id' => 'psts_poster_id', + 'topic_id' => 'psts_topic_id', + ], + $this->table_prefix . 'privmsgs_folder' => [ + 'user_id' => 'pmfldr_user_id', + ], + $this->table_prefix . 'privmsgs_rules' => [ + 'user_id' => 'pmrls_user_id', + ], + $this->table_prefix . 'topics' => [ + 'forum_id' => 'tpcs_forum_id', + ], + $this->table_prefix . 'topics_track' => [ + 'forum_id' => 'tpcstrk_forum_id', + 'topic_id' => 'tpcstrk_topic_id', + ], + $this->table_prefix . 'topics_watch' => [ + 'topic_id' => 'tpcswtch_topic_id', + 'user_id' => 'tpcswtch_user_id', + ], + $this->table_prefix . 'user_group' => [ + 'group_id' => 'usrgrp_group_id', + 'user_id' => 'usrgrp_user_id', + ], + $this->table_prefix . 'user_notifications' => [ + 'user_id' => 'usrntf_user_id', ], ], ]; @@ -42,17 +102,11 @@ class rename_auth_role_id_index extends migration public function revert_schema() { - return [ - 'drop_keys' => [ - $this->table_prefix . 'acl_users' => [ - 'usr_auth_role_id', - ], - ], - 'add_index' => [ - $this->table_prefix . 'acl_users' => [ - 'auth_role_id' => ['auth_role_id'], - ], - ], - ]; + $schema = $this->update_schema(); + array_walk($schema['rename_index'], function (&$index_data, $table_name) { + $index_data = array_flip($index_data); + }); + + return $schema; } } diff --git a/phpBB/phpbb/db/migration/helper.php b/phpBB/phpbb/db/migration/helper.php index bce2efff51..44cd23462a 100644 --- a/phpBB/phpbb/db/migration/helper.php +++ b/phpBB/phpbb/db/migration/helper.php @@ -42,6 +42,7 @@ class helper 'add_primary_keys' => 2, // perform_schema_changes only uses one level, but second is in the function 'add_unique_index' => 2, 'add_index' => 2, + 'rename_index' => 1, ); foreach ($nested_level as $change_type => $data_depth) diff --git a/phpBB/phpbb/db/migration/schema_generator.php b/phpBB/phpbb/db/migration/schema_generator.php index ce07c30d45..90cfddbe5a 100644 --- a/phpBB/phpbb/db/migration/schema_generator.php +++ b/phpBB/phpbb/db/migration/schema_generator.php @@ -187,6 +187,7 @@ class schema_generator 'add_index' => 'KEYS', 'add_unique_index' => 'KEYS', 'drop_keys' => 'KEYS', + 'rename_index' => 'KEYS', ]; $schema_changes = $migration->update_schema(); @@ -206,6 +207,7 @@ class schema_generator { case 'add': case 'change': + case 'rename': $action = function(&$value, $changes, $value_transform = null) { self::set_all($value, $changes, $value_transform); }; @@ -347,7 +349,7 @@ class schema_generator */ private static function get_value_transform(string $change_type, string $schema_type) : Closure|null { - if ($change_type !== 'add') + if (!in_array($change_type, ['add', 'rename'])) { return null; } @@ -355,6 +357,23 @@ class schema_generator switch ($schema_type) { case 'index': + if ($change_type == 'rename') + { + return function(&$value, $key, $change) { + if (isset($value[$key])) + { + $data_backup = $value[$key]; + unset($value[$key]); + $value[$change] = $data_backup; + unset($data_backup); + } + else + { + return null; + } + }; + } + return function(&$value, $key, $change) { $value[$key] = ['INDEX', $change]; }; diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 65fd6b39aa..73987d26ee 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -330,6 +330,19 @@ class doctrine implements tools_interface ); } + /** + * {@inheritDoc} + */ + public function sql_rename_index(string $table_name, string $index_name_old, string $index_name_new) + { + return $this->alter_schema( + function (Schema $schema) use ($table_name, $index_name_old, $index_name_new): void + { + $this->schema_rename_index($schema, $table_name, $index_name_old, $index_name_new); + } + ); + } + /** * {@inheritDoc} */ @@ -545,6 +558,11 @@ class doctrine implements tools_interface 'use_key' => true, 'per_table' => true, ], + 'rename_index' => [ + 'method' => 'schema_rename_index', + 'use_key' => true, + 'per_table' => true, + ], ]; foreach ($mapping as $action => $params) @@ -831,6 +849,27 @@ class doctrine implements tools_interface $table->addIndex($columns, $index_name); } + /** + * @param Schema $schema + * @param string $table_name + * @param string $index_name_old + * @param string $index_name_new + * @param bool $safe_check + * + * @throws SchemaException + */ + protected function schema_rename_index(Schema $schema, string $table_name, string $index_name_old, string $index_name_new, bool $safe_check = false): void + { + $table = $schema->getTable($table_name); + + if ($safe_check && !$table->hasIndex($index_name_old)) + { + return; + } + + $table->renameIndex($index_name_old, $index_name_new); + } + /** * @param Schema $schema * @param string $table_name diff --git a/phpBB/phpbb/db/tools/tools_interface.php b/phpBB/phpbb/db/tools/tools_interface.php index 0b8cefd8dd..09f47ed566 100644 --- a/phpBB/phpbb/db/tools/tools_interface.php +++ b/phpBB/phpbb/db/tools/tools_interface.php @@ -165,6 +165,17 @@ interface tools_interface */ public function sql_create_index(string $table_name, string $index_name, $column); + /** + * Rename index + * + * @param string $table_name Table to modify + * @param string $index_name_old Name of the index to rename + * @param string $index_name_new New name of the index being renamed + * + * @return bool|string[] True if the statements have been executed + */ + public function sql_rename_index(string $table_name, string $index_name_old, string $index_name_new); + /** * Drop Index * From 78dcb0c86207b182d8abd517880e5057ca3a3f85 Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 17 Jun 2025 01:01:19 +0700 Subject: [PATCH 04/22] [ticket/17525] Avoid more index name duplication PHPBB-17525 --- .../data/v400/rename_auth_role_id_index.php | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php b/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php index 9a3fd790f7..9537c884dd 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php @@ -40,8 +40,15 @@ class rename_auth_role_id_index extends migration 'poster_id' => 'attchmnts_poster_id', 'topic_id' => 'attchmnts_topic_id', ], + $this->table_prefix . 'bbcodes' => [ + 'display_on_post' => 'bbcds_display_on_post', + ], + $this->table_prefix . 'forums' => [ + 'left_right_id' => 'frms_left_right_id', + ], $this->table_prefix . 'forums_watch' => [ 'forum_id' => 'frmswtch_forum_id', + 'notify_stat' => 'frmswtch_notify_stat', 'user_id' => 'frmswtch_user_id', ], $this->table_prefix . 'log' => [ @@ -55,10 +62,15 @@ class rename_auth_role_id_index extends migration $this->table_prefix . 'moderator_cache' => [ 'forum_id' => 'mdrtrcch_forum_id', ], + $this->table_prefix . 'modules' => [ + 'left_right_id' => 'mdls_left_right_id', + ], $this->table_prefix . 'oauth_states' => [ + 'provider' => 'oauthsts_provider', 'user_id' => 'oauthsts_user_id', ], $this->table_prefix . 'oauth_tokens' => [ + 'provider' => 'oauthtkns_provider', 'user_id' => 'oauthtkns_user_id', ], $this->table_prefix . 'poll_options' => [ @@ -72,11 +84,26 @@ class rename_auth_role_id_index extends migration 'poster_id' => 'psts_poster_id', 'topic_id' => 'psts_topic_id', ], + $this->table_prefix . 'privmsgs' => [ + 'author_id' => 'pms_author_id', + ], $this->table_prefix . 'privmsgs_folder' => [ - 'user_id' => 'pmfldr_user_id', + 'user_id' => 'pmsfldr_user_id', ], $this->table_prefix . 'privmsgs_rules' => [ - 'user_id' => 'pmrls_user_id', + 'user_id' => 'pmsrls_user_id', + ], + $this->table_prefix . 'privmsgs_to' => [ + 'author_id' => 'pmsto_author_id', + ], + $this->table_prefix . 'reports' => [ + 'post_id' => 'rprts_post_id', + ], + $this->table_prefix . 'search_wordmatch' => [ + 'post_id' => 'wrdmtch_post_id', + ], + $this->table_prefix . 'smilies' => [ + 'display_on_post' => 'smls_display_on_post', ], $this->table_prefix . 'topics' => [ 'forum_id' => 'tpcs_forum_id', @@ -87,6 +114,7 @@ class rename_auth_role_id_index extends migration ], $this->table_prefix . 'topics_watch' => [ 'topic_id' => 'tpcswtch_topic_id', + 'notify_stat' => 'tpcswtch_notify_stat', 'user_id' => 'tpcswtch_user_id', ], $this->table_prefix . 'user_group' => [ From 84d195ac218bfc09c76f588131cb921864a614ca Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 17 Jun 2025 12:14:01 +0700 Subject: [PATCH 05/22] [ticket/17525] Add migration renaming index test PHPBB-17525 --- tests/dbal/migration/schema_index.php | 65 +++++++++++++++++++++++++++ tests/dbal/migrator_test.php | 24 ++++++++++ 2 files changed, 89 insertions(+) create mode 100644 tests/dbal/migration/schema_index.php diff --git a/tests/dbal/migration/schema_index.php b/tests/dbal/migration/schema_index.php new file mode 100644 index 0000000000..1ea8eb7c78 --- /dev/null +++ b/tests/dbal/migration/schema_index.php @@ -0,0 +1,65 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +class phpbb_dbal_migration_schema_index extends \phpbb\db\migration\migration +{ + function update_schema() + { + return [ + 'add_tables' => [ + $this->table_prefix . 'foobar1' => [ + 'COLUMNS' => [ + 'user_id' => ['UINT', 0], + 'username' => ['VCHAR:50', 0], + ], + 'KEYS' => [ + 'tstidx_user_id' => ['UNIQUE', 'user_id'], + 'tstidx_username' => ['INDEX', 'username'], + ], + ], + $this->table_prefix . 'foobar2' => [ + 'COLUMNS' => [ + 'ban_userid' => ['ULINT', 0], + 'ban_ip' => ['VCHAR:40', ''], + 'ban_reason' => ['VCHAR:100', ''], + ], + 'KEYS' => [ + 'tstidx_ban_userid' => ['UNIQUE', 'ban_userid'], + 'tstidxban_data' => ['INDEX', ['ban_ip', 'ban_reason']], + ], + ], + ], + + 'rename_index' => [ + $this->table_prefix . 'foobar1' => [ + 'tstidx_user_id' => 'fbr1_user_id', + 'tstidx_username' => 'fbr1_username', + ], + $this->table_prefix . 'foobar2' => [ + 'tstidx_ban_userid' => 'fbr2_ban_userid', + 'tstidxban_data' => 'fbr2_ban_data', + ], + ], + ]; + } + + function revert_schema() + { + return [ + 'drop_tables' => [ + $this->table_prefix . 'foobar1', + $this->table_prefix . 'foobar2', + ], + ]; + } +} diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 5d16fa63ee..f48ad35275 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -24,6 +24,7 @@ require_once __DIR__ . '/migration/revert_table_with_dependency.php'; require_once __DIR__ . '/migration/fail.php'; require_once __DIR__ . '/migration/installed.php'; require_once __DIR__ . '/migration/schema.php'; +require_once __DIR__ . '/migration/schema_index.php'; class phpbb_dbal_migrator_test extends phpbb_database_test_case { @@ -416,4 +417,27 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertFalse($this->db_tools->sql_column_exists('phpbb_config', 'test_column1')); $this->assertFalse($this->db_tools->sql_table_exists('phpbb_foobar')); } + + public function test_rename_index() + { + $this->migrator->set_migrations(array('phpbb_dbal_migration_schema_index')); + + while (!$this->migrator->finished()) + { + $this->migrator->update(); + } + + $this->assertTrue($this->db_tools->sql_unique_index_exists('phpbb_foobar1', 'fbr1_user_id')); + $this->assertTrue($this->db_tools->sql_index_exists('phpbb_foobar1', 'fbr1_username')); + $this->assertTrue($this->db_tools->sql_unique_index_exists('phpbb_foobar2', 'fbr2_ban_userid')); + $this->assertTrue($this->db_tools->sql_index_exists('phpbb_foobar2', 'fbr2_ban_data')); + + while ($this->migrator->migration_state('phpbb_dbal_migration_schema_index')) + { + $this->migrator->revert('phpbb_dbal_migration_schema_index'); + } + + $this->assertFalse($this->db_tools->sql_table_exists('phpbb_foobar1')); + $this->assertFalse($this->db_tools->sql_table_exists('phpbb_foobar2')); + } } From 45a69eca140895981b41fc8b4ca2b6870aad8cfc Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 17 Jun 2025 14:09:34 +0700 Subject: [PATCH 06/22] [ticket/17525] Rename migration to reflect its purpose PHPBB-17525 --- ...auth_role_id_index.php => rename_duplicated_index_names.php} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename phpBB/phpbb/db/migration/data/v400/{rename_auth_role_id_index.php => rename_duplicated_index_names.php} (98%) diff --git a/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php similarity index 98% rename from phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php rename to phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index 9537c884dd..0c55bd3854 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_auth_role_id_index.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -15,7 +15,7 @@ namespace phpbb\db\migration\data\v400; use phpbb\db\migration\migration; -class rename_auth_role_id_index extends migration +class rename_duplicated_index_names extends migration { public static function depends_on() { From 1339a31c2369ed545dc086873e644bb28b5bce1b Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 25 Jun 2025 17:42:36 +0700 Subject: [PATCH 07/22] [ticket/17525] Rename all indexes to make names unique With this reNAMING schema, max index name length is 23. PHPBB-17525 --- phpBB/phpbb/db/doctrine/table_helper.php | 102 ++++++++++++++ .../v400/rename_duplicated_index_names.php | 125 ++++-------------- 2 files changed, 129 insertions(+), 98 deletions(-) diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index 935db35838..98d33b80cd 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -123,6 +123,108 @@ class table_helper } } + /** + * Maps short table names for the purpose of prefixing tables' index names. + * + * @param array $additional_tables Additional table names without prefix to add to the map. + * @param array $table_prefix Tables prefix. + * + * @return array Pairs of table names and their short name representations. + */ + public static function map_short_table_names(array $additional_tables = [], string $table_prefix = ''): array + { + $short_table_names_map = [ + "{$table_prefix}acl_groups" => 'aclgrps', + "{$table_prefix}acl_options" => 'aclopts', + "{$table_prefix}acl_roles" => 'aclrls', + "{$table_prefix}acl_roles_data" => 'aclrlsdt', + "{$table_prefix}acl_users" => 'aclusrs', + "{$table_prefix}attachments" => 'atchmnts', + "{$table_prefix}backups" => 'bckps', + "{$table_prefix}bans" => 'bans', + "{$table_prefix}bbcodes" => 'bbcds', + "{$table_prefix}bookmarks" => 'bkmrks', + "{$table_prefix}bots" => 'bots', + "{$table_prefix}captcha_answers" => 'cptchans', + "{$table_prefix}captcha_questions" => 'cptchqs', + "{$table_prefix}config" => 'cnfg', + "{$table_prefix}config_text" => 'cnfgtxt', + "{$table_prefix}confirm" => 'cnfrm', + "{$table_prefix}disallow" => 'dslw', + "{$table_prefix}drafts" => 'drfts', + "{$table_prefix}ext" => 'ext', + "{$table_prefix}extension_groups" => 'extgrps', + "{$table_prefix}extensions" => 'exts', + "{$table_prefix}forums" => 'frms', + "{$table_prefix}forums_access" => 'frmsacs', + "{$table_prefix}forums_track" => 'frmstrck', + "{$table_prefix}forums_watch" => 'frmswtch', + "{$table_prefix}groups" => 'grps', + "{$table_prefix}icons" => 'icns', + "{$table_prefix}lang" => 'lang', + "{$table_prefix}log" => 'log', + "{$table_prefix}login_attempts" => 'lgnatmpts', + "{$table_prefix}migrations" => 'mgrtns', + "{$table_prefix}moderator_cache" => 'mdrtche', + "{$table_prefix}modules" => 'mdls', + "{$table_prefix}notification_emails"=> 'ntfemls', + "{$table_prefix}notification_push" => 'ntfpsh', + "{$table_prefix}notification_types" => 'ntftps', + "{$table_prefix}notifications" => 'nftcns', + "{$table_prefix}oauth_accounts" => 'oauthacnts', + "{$table_prefix}oauth_states" => 'oauthsts', + "{$table_prefix}oauth_tokens" => 'oauthtkns', + "{$table_prefix}poll_options" => 'pllopts', + "{$table_prefix}poll_votes" => 'pllvts', + "{$table_prefix}posts" => 'psts', + "{$table_prefix}privmsgs" => 'pms', + "{$table_prefix}privmsgs_folder" => 'pmsfldr', + "{$table_prefix}privmsgs_rules" => 'pmsrls', + "{$table_prefix}privmsgs_to" => 'pmsto', + "{$table_prefix}profile_fields" => 'prflds', + "{$table_prefix}profile_fields_data"=> 'prfldt', + "{$table_prefix}profile_fields_lang"=> 'prfldlng', + "{$table_prefix}profile_lang" => 'prflng', + "{$table_prefix}push_subscriptions" => 'pshsbscrs', + "{$table_prefix}qa_confirm" => 'qacnfm', + "{$table_prefix}ranks" => 'rnks', + "{$table_prefix}reports" => 'rprts', + "{$table_prefix}reports_reasons" => 'rprtrsns', + "{$table_prefix}search_results" => 'srchrslts', + "{$table_prefix}search_wordlist" => 'wrdlst', + "{$table_prefix}search_wordmatch" => 'wrdmtch', + "{$table_prefix}sessions" => 'ssns', + "{$table_prefix}sessions_keys" => 'ssnkeys', + "{$table_prefix}sitelist" => 'sitelst', + "{$table_prefix}smilies" => 'smls', + "{$table_prefix}storage" => 'strg', + "{$table_prefix}styles" => 'stls', + "{$table_prefix}teampage" => 'teampg', + "{$table_prefix}topics" => 'tpcs', + "{$table_prefix}topics_posted" => 'tpcspstd', + "{$table_prefix}topics_track" => 'tpcstrk', + "{$table_prefix}topics_watch" => 'tpkswtch', + "{$table_prefix}user_group" => 'usrgrp', + "{$table_prefix}user_notifications" => 'usrntfs', + "{$table_prefix}users" => 'usrs', + "{$table_prefix}warnings" => 'wrns', + "{$table_prefix}words" => 'wrds', + "{$table_prefix}zebra" => 'zbra', + ]; + + // Add table prefix to additional tables + if (!empty($table_prefix && !empty($additional_tables))) + { + foreach ($additional_tables as $key => $value) + { + $additional_tables["{$table_prefix}{$key}"] = $value; + unset($additional_tables[$key]); + } + } + + return array_merge($short_table_names_map, $additional_tables); + } + /** * Private constructor. Call methods of table_helper statically. */ diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index 0c55bd3854..60900f67ae 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -14,6 +14,7 @@ namespace phpbb\db\migration\data\v400; use phpbb\db\migration\migration; +use phpbb\db\doctrine\table_helper; class rename_duplicated_index_names extends migration { @@ -26,105 +27,33 @@ class rename_duplicated_index_names extends migration public function update_schema() { + $rename_index = $table_keys = []; + $db_table_schema = json_decode(file_get_contents($this->phpbb_root_path . 'store/schema.json'), true); + foreach ($db_table_schema as $table_name => $table_data) + { + if (isset($table_data['KEYS'])) + { + $table_name = $this->table_prefix . $table_name; + foreach ($table_data['KEYS'] as $key_name => $key_data) + { + $table_keys[$table_name][] = $key_name; + } + } + } + + $short_table_names = table_helper::map_short_table_names([], $this->table_prefix); + foreach ($table_keys as $table_name => $key_names) + { + $key_name_new = $short_table_names[$table_name] . '_' . $key_name; + foreach ($key_names as $key_name) + { + $rename_index[$table_name][$key_name] = $key_name_new; + $rename_index[$table_name][$table_name . '_' . $key_name] = $key_name_new; + } + } + return [ - 'rename_index' => [ - $this->table_prefix . 'acl_groups' => [ - 'auth_role_id' => 'aclgrps_auth_role_id', - 'group_id' => 'aclgrps_group_id', - ], - $this->table_prefix . 'acl_users' => [ - 'auth_role_id' => 'aclusrs_auth_role_id', - 'user_id' => 'aclusrs_user_id', - ], - $this->table_prefix . 'attachments' => [ - 'poster_id' => 'attchmnts_poster_id', - 'topic_id' => 'attchmnts_topic_id', - ], - $this->table_prefix . 'bbcodes' => [ - 'display_on_post' => 'bbcds_display_on_post', - ], - $this->table_prefix . 'forums' => [ - 'left_right_id' => 'frms_left_right_id', - ], - $this->table_prefix . 'forums_watch' => [ - 'forum_id' => 'frmswtch_forum_id', - 'notify_stat' => 'frmswtch_notify_stat', - 'user_id' => 'frmswtch_user_id', - ], - $this->table_prefix . 'log' => [ - 'forum_id' => 'log_forum_id', - 'topic_id' => 'log_topic_id', - 'user_id' => 'log_user_id', - ], - $this->table_prefix . 'login_attempts' => [ - 'user_id' => 'lgnatmpts_user_id', - ], - $this->table_prefix . 'moderator_cache' => [ - 'forum_id' => 'mdrtrcch_forum_id', - ], - $this->table_prefix . 'modules' => [ - 'left_right_id' => 'mdls_left_right_id', - ], - $this->table_prefix . 'oauth_states' => [ - 'provider' => 'oauthsts_provider', - 'user_id' => 'oauthsts_user_id', - ], - $this->table_prefix . 'oauth_tokens' => [ - 'provider' => 'oauthtkns_provider', - 'user_id' => 'oauthtkns_user_id', - ], - $this->table_prefix . 'poll_options' => [ - 'topic_id' => 'pllopts_topic_id', - ], - $this->table_prefix . 'poll_votes' => [ - 'topic_id' => 'pllvts_topic_id', - ], - $this->table_prefix . 'posts' => [ - 'forum_id' => 'psts_forum_id', - 'poster_id' => 'psts_poster_id', - 'topic_id' => 'psts_topic_id', - ], - $this->table_prefix . 'privmsgs' => [ - 'author_id' => 'pms_author_id', - ], - $this->table_prefix . 'privmsgs_folder' => [ - 'user_id' => 'pmsfldr_user_id', - ], - $this->table_prefix . 'privmsgs_rules' => [ - 'user_id' => 'pmsrls_user_id', - ], - $this->table_prefix . 'privmsgs_to' => [ - 'author_id' => 'pmsto_author_id', - ], - $this->table_prefix . 'reports' => [ - 'post_id' => 'rprts_post_id', - ], - $this->table_prefix . 'search_wordmatch' => [ - 'post_id' => 'wrdmtch_post_id', - ], - $this->table_prefix . 'smilies' => [ - 'display_on_post' => 'smls_display_on_post', - ], - $this->table_prefix . 'topics' => [ - 'forum_id' => 'tpcs_forum_id', - ], - $this->table_prefix . 'topics_track' => [ - 'forum_id' => 'tpcstrk_forum_id', - 'topic_id' => 'tpcstrk_topic_id', - ], - $this->table_prefix . 'topics_watch' => [ - 'topic_id' => 'tpcswtch_topic_id', - 'notify_stat' => 'tpcswtch_notify_stat', - 'user_id' => 'tpcswtch_user_id', - ], - $this->table_prefix . 'user_group' => [ - 'group_id' => 'usrgrp_group_id', - 'user_id' => 'usrgrp_user_id', - ], - $this->table_prefix . 'user_notifications' => [ - 'user_id' => 'usrntf_user_id', - ], - ], + 'rename_index' => $rename_index, ]; } From a229797cd756e6b05a735709ca13649a2fb3cf86 Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 25 Jun 2025 20:52:45 +0700 Subject: [PATCH 08/22] [ticket/17525] Add database schema getter PHPBB-17525 --- phpBB/phpbb/db/doctrine/table_helper.php | 3 ++- .../v400/rename_duplicated_index_names.php | 26 +++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index 98d33b80cd..448597df3d 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -130,11 +130,12 @@ class table_helper * @param array $table_prefix Tables prefix. * * @return array Pairs of table names and their short name representations. + * @psalm-return array{string, string} */ public static function map_short_table_names(array $additional_tables = [], string $table_prefix = ''): array { $short_table_names_map = [ - "{$table_prefix}acl_groups" => 'aclgrps', + "{$table_prefix}acl_groups" => 'aclgrps', "{$table_prefix}acl_options" => 'aclopts', "{$table_prefix}acl_roles" => 'aclrls', "{$table_prefix}acl_roles_data" => 'aclrlsdt', diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index 60900f67ae..e3924c1c9d 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -28,12 +28,11 @@ class rename_duplicated_index_names extends migration public function update_schema() { $rename_index = $table_keys = []; - $db_table_schema = json_decode(file_get_contents($this->phpbb_root_path . 'store/schema.json'), true); + $db_table_schema = $this->get_schema(); foreach ($db_table_schema as $table_name => $table_data) { if (isset($table_data['KEYS'])) { - $table_name = $this->table_prefix . $table_name; foreach ($table_data['KEYS'] as $key_name => $key_data) { $table_keys[$table_name][] = $key_name; @@ -66,4 +65,27 @@ class rename_duplicated_index_names extends migration return $schema; } + + protected function get_schema() + { + $self_classname = '\\' . str_replace('/', '\\', self::class); + $finder_factory = new \phpbb\finder\factory(null, false, $this->phpbb_root_path, $this->php_ext); + $finder = $finder_factory->get(); + $migrator_classes = $finder->core_path('phpbb/db/migration/data/')->get_classes(); + $self_class_index = array_search($self_classname, $migrator_classes); + unset($migrator_classes[$self_class_index]); + + $schema_generator = new \phpbb\db\migration\schema_generator( + $migrator_classes, + $this->config, + $this->db, + $this->db_tools, + $this->phpbb_root_path, + $this->php_ext, + $this->table_prefix, + $this->tables + ); + + return $schema_generator->get_schema(); + } } From 75c5fe9459260e91e8887cc68e845865a0d6490b Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 25 Jun 2025 22:24:50 +0700 Subject: [PATCH 09/22] [ticket/17525] Add index names test for generated database schema PHPBB-17525 --- phpBB/phpbb/db/doctrine/table_helper.php | 2 +- tests/dbal/migrator_test.php | 55 ++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index 448597df3d..70ac39078f 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -127,7 +127,7 @@ class table_helper * Maps short table names for the purpose of prefixing tables' index names. * * @param array $additional_tables Additional table names without prefix to add to the map. - * @param array $table_prefix Tables prefix. + * @param string $table_prefix Tables prefix. * * @return array Pairs of table names and their short name representations. * @psalm-return array{string, string} diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index f48ad35275..ac502f1816 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -440,4 +440,59 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertFalse($this->db_tools->sql_table_exists('phpbb_foobar1')); $this->assertFalse($this->db_tools->sql_table_exists('phpbb_foobar2')); } + + public function test_schema_generator(): array + { + global $phpbb_root_path, $phpEx; + + $finder_factory = new \phpbb\finder\factory(null, false, $phpbb_root_path, $phpEx); + $finder = $finder_factory->get(); + $migrator_classes = $finder->core_path('phpbb/db/migration/data/')->get_classes(); + + $schema_generator = new \phpbb\db\migration\schema_generator( + $migrator_classes, + $this->config, + $this->db, + $this->db_tools, + $phpbb_root_path, + $phpEx, + 'phpbb_', + self::get_core_tables() + ); + $db_table_schema = $schema_generator->get_schema(); + + $this->assertNotEmpty($db_table_schema); + + return $db_table_schema; + } + + /** + * @depends test_schema_generator + */ + public function test_table_indexes(array $db_table_schema) + { + $table_keys = []; + foreach ($db_table_schema as $table_name => $table_data) + { + if (isset($table_data['KEYS'])) + { + foreach ($table_data['KEYS'] as $key_name => $key_data) + { + $table_keys[$table_name][] = $key_name; + } + } + } + + $this->assertNotEmpty($table_keys); + + $short_table_names = \phpbb\db\doctrine\table_helper::map_short_table_names([], 'phpbb_'); + foreach ($table_keys as $table_name => $key_names) + { + $index_prefix = $short_table_names[$table_name] . '_'; + foreach ($key_names as $key_name) + { + $this->assertFalse(strpos($key_name, $index_prefix), "$key_name does not contain $index_prefix"); + } + } + } } From de1f6329ff7dae141bb0073a0be8604a5c7dd525 Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 25 Jun 2025 22:44:08 +0700 Subject: [PATCH 10/22] [ticket/17525] Fix tests PHPBB-17525 --- phpBB/phpbb/db/doctrine/table_helper.php | 2 +- tests/dbal/migrator_test.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index 70ac39078f..ec5c2040e8 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -214,7 +214,7 @@ class table_helper ]; // Add table prefix to additional tables - if (!empty($table_prefix && !empty($additional_tables))) + if (!empty($table_prefix) && !empty($additional_tables)) { foreach ($additional_tables as $key => $value) { diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index ac502f1816..454c05b5eb 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -491,7 +491,7 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $index_prefix = $short_table_names[$table_name] . '_'; foreach ($key_names as $key_name) { - $this->assertFalse(strpos($key_name, $index_prefix), "$key_name does not contain $index_prefix"); + $this->assertEquals(0, strpos($key_name, $index_prefix), "$key_name does not contain $index_prefix"); } } } From 5e9d616f5760812cd5ed6b63dca0fefb4adf0b78 Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 25 Jun 2025 23:48:45 +0700 Subject: [PATCH 11/22] [ticket/17525] Map Sphinx table, add more test assertions PHPBB-17525 --- phpBB/phpbb/db/doctrine/table_helper.php | 3 ++- tests/dbal/migrator_test.php | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index ec5c2040e8..972d31eab4 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -198,12 +198,13 @@ class table_helper "{$table_prefix}sessions_keys" => 'ssnkeys', "{$table_prefix}sitelist" => 'sitelst', "{$table_prefix}smilies" => 'smls', + "{$table_prefix}sphinx" => 'sphnx', "{$table_prefix}storage" => 'strg', "{$table_prefix}styles" => 'stls', "{$table_prefix}teampage" => 'teampg', "{$table_prefix}topics" => 'tpcs', "{$table_prefix}topics_posted" => 'tpcspstd', - "{$table_prefix}topics_track" => 'tpcstrk', + "{$table_prefix}topics_track" => 'tpcstrck', "{$table_prefix}topics_watch" => 'tpkswtch', "{$table_prefix}user_group" => 'usrgrp', "{$table_prefix}user_notifications" => 'usrntfs', diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 454c05b5eb..e7fbd4eb2d 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -485,7 +485,10 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertNotEmpty($table_keys); - $short_table_names = \phpbb\db\doctrine\table_helper::map_short_table_names([], 'phpbb_'); + $short_table_names = \phpbb\db\doctrine\table_helper::map_short_table_names(['custom_table' => 'cstmtbl'], 'phpbb_'); + $this->assertEquals('phpbb_custom_table', array_search('cstmtbl', $short_table_names)); + $this->assertEquals($short_table_names['phpbb_custom_table'], 'cstmtbl'); + foreach ($table_keys as $table_name => $key_names) { $index_prefix = $short_table_names[$table_name] . '_'; From 4da8591dd8a47d917ed2aa39830c71d71cb5f718 Mon Sep 17 00:00:00 2001 From: rxu Date: Fri, 27 Jun 2025 22:10:27 +0700 Subject: [PATCH 12/22] [ticket/17525] Run index renaming migration the last in migrations queue PHPBB-17525 --- .../db/migration/data/v400/rename_duplicated_index_names.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index e3924c1c9d..0ca9359a47 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -21,7 +21,7 @@ class rename_duplicated_index_names extends migration public static function depends_on() { return [ - '\phpbb\db\migration\data\v400\dev', + '\phpbb\db\migration\data\v400\storage_track_index', ]; } @@ -43,9 +43,9 @@ class rename_duplicated_index_names extends migration $short_table_names = table_helper::map_short_table_names([], $this->table_prefix); foreach ($table_keys as $table_name => $key_names) { - $key_name_new = $short_table_names[$table_name] . '_' . $key_name; foreach ($key_names as $key_name) { + $key_name_new = $short_table_names[$table_name] . '_' . $key_name; $rename_index[$table_name][$key_name] = $key_name_new; $rename_index[$table_name][$table_name . '_' . $key_name] = $key_name_new; } From 58b3e5dee04f26f5a160e765691fe7401ee0e0db Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 1 Jul 2025 00:11:36 +0700 Subject: [PATCH 13/22] [ticket/17525] Migration to rename actual database indexes if any Also generate short table names rather than use hardcoded map PHPBB-17525 --- phpBB/phpbb/db/doctrine/table_helper.php | 128 +++++------------- .../v400/rename_duplicated_index_names.php | 77 ++++++++--- 2 files changed, 94 insertions(+), 111 deletions(-) diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index 972d31eab4..150e98d95b 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -124,107 +124,51 @@ class table_helper } /** - * Maps short table names for the purpose of prefixing tables' index names. + * Maps table names to thair short names for the purpose of prefixing tables' index names. * - * @param array $additional_tables Additional table names without prefix to add to the map. - * @param string $table_prefix Tables prefix. + * @param array $table_names Table names with prefix to add to the map. + * @param string $table_prefix Tables prefix. * * @return array Pairs of table names and their short name representations. * @psalm-return array{string, string} */ - public static function map_short_table_names(array $additional_tables = [], string $table_prefix = ''): array + public static function map_short_table_names(array $table_names = [], string $table_prefix = ''): array { - $short_table_names_map = [ - "{$table_prefix}acl_groups" => 'aclgrps', - "{$table_prefix}acl_options" => 'aclopts', - "{$table_prefix}acl_roles" => 'aclrls', - "{$table_prefix}acl_roles_data" => 'aclrlsdt', - "{$table_prefix}acl_users" => 'aclusrs', - "{$table_prefix}attachments" => 'atchmnts', - "{$table_prefix}backups" => 'bckps', - "{$table_prefix}bans" => 'bans', - "{$table_prefix}bbcodes" => 'bbcds', - "{$table_prefix}bookmarks" => 'bkmrks', - "{$table_prefix}bots" => 'bots', - "{$table_prefix}captcha_answers" => 'cptchans', - "{$table_prefix}captcha_questions" => 'cptchqs', - "{$table_prefix}config" => 'cnfg', - "{$table_prefix}config_text" => 'cnfgtxt', - "{$table_prefix}confirm" => 'cnfrm', - "{$table_prefix}disallow" => 'dslw', - "{$table_prefix}drafts" => 'drfts', - "{$table_prefix}ext" => 'ext', - "{$table_prefix}extension_groups" => 'extgrps', - "{$table_prefix}extensions" => 'exts', - "{$table_prefix}forums" => 'frms', - "{$table_prefix}forums_access" => 'frmsacs', - "{$table_prefix}forums_track" => 'frmstrck', - "{$table_prefix}forums_watch" => 'frmswtch', - "{$table_prefix}groups" => 'grps', - "{$table_prefix}icons" => 'icns', - "{$table_prefix}lang" => 'lang', - "{$table_prefix}log" => 'log', - "{$table_prefix}login_attempts" => 'lgnatmpts', - "{$table_prefix}migrations" => 'mgrtns', - "{$table_prefix}moderator_cache" => 'mdrtche', - "{$table_prefix}modules" => 'mdls', - "{$table_prefix}notification_emails"=> 'ntfemls', - "{$table_prefix}notification_push" => 'ntfpsh', - "{$table_prefix}notification_types" => 'ntftps', - "{$table_prefix}notifications" => 'nftcns', - "{$table_prefix}oauth_accounts" => 'oauthacnts', - "{$table_prefix}oauth_states" => 'oauthsts', - "{$table_prefix}oauth_tokens" => 'oauthtkns', - "{$table_prefix}poll_options" => 'pllopts', - "{$table_prefix}poll_votes" => 'pllvts', - "{$table_prefix}posts" => 'psts', - "{$table_prefix}privmsgs" => 'pms', - "{$table_prefix}privmsgs_folder" => 'pmsfldr', - "{$table_prefix}privmsgs_rules" => 'pmsrls', - "{$table_prefix}privmsgs_to" => 'pmsto', - "{$table_prefix}profile_fields" => 'prflds', - "{$table_prefix}profile_fields_data"=> 'prfldt', - "{$table_prefix}profile_fields_lang"=> 'prfldlng', - "{$table_prefix}profile_lang" => 'prflng', - "{$table_prefix}push_subscriptions" => 'pshsbscrs', - "{$table_prefix}qa_confirm" => 'qacnfm', - "{$table_prefix}ranks" => 'rnks', - "{$table_prefix}reports" => 'rprts', - "{$table_prefix}reports_reasons" => 'rprtrsns', - "{$table_prefix}search_results" => 'srchrslts', - "{$table_prefix}search_wordlist" => 'wrdlst', - "{$table_prefix}search_wordmatch" => 'wrdmtch', - "{$table_prefix}sessions" => 'ssns', - "{$table_prefix}sessions_keys" => 'ssnkeys', - "{$table_prefix}sitelist" => 'sitelst', - "{$table_prefix}smilies" => 'smls', - "{$table_prefix}sphinx" => 'sphnx', - "{$table_prefix}storage" => 'strg', - "{$table_prefix}styles" => 'stls', - "{$table_prefix}teampage" => 'teampg', - "{$table_prefix}topics" => 'tpcs', - "{$table_prefix}topics_posted" => 'tpcspstd', - "{$table_prefix}topics_track" => 'tpcstrck', - "{$table_prefix}topics_watch" => 'tpkswtch', - "{$table_prefix}user_group" => 'usrgrp', - "{$table_prefix}user_notifications" => 'usrntfs', - "{$table_prefix}users" => 'usrs', - "{$table_prefix}warnings" => 'wrns', - "{$table_prefix}words" => 'wrds', - "{$table_prefix}zebra" => 'zbra', - ]; - - // Add table prefix to additional tables - if (!empty($table_prefix) && !empty($additional_tables)) + $short_table_names_map = []; + foreach ($table_names as $table_name) { - foreach ($additional_tables as $key => $value) - { - $additional_tables["{$table_prefix}{$key}"] = $value; - unset($additional_tables[$key]); - } + $short_table_names_map[$table_name] = self::generate_shortname(str_replace($table_prefix, '', $table_name)); } - return array_merge($short_table_names_map, $additional_tables); + return $short_table_names_map; + } + + /** + * Generates short table names for the purpose of prefixing tables' index names. + * + * @param string $table_name Table name with prefix to generate its short name. + * + * @return string Short table name. + */ + public static function generate_shortname(string $table_name = ''): string + { + // Only shorten actually long names + if (strlen($table_name) > 4) + { + // Remove vowels + $table_name = preg_replace('/([^aeiou_])([aeiou]+)/i', '$1', $table_name); + + // Remove underscores + $table_name = str_replace('_', '', $table_name); + + // Remove repeated consonants and their combinations (like 'ss', 'flfl' and similar) + $table_name = preg_replace('/(.+)\\1+/i', '$1', $table_name); + + // Restrict short name length to 10 chars + $table_name = substr($table_name, 0, 10); + } + + return $table_name; } /** diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index 0ca9359a47..eb785878fb 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -13,10 +13,10 @@ namespace phpbb\db\migration\data\v400; -use phpbb\db\migration\migration; +use phpbb\db\migration\container_aware_migration; use phpbb\db\doctrine\table_helper; -class rename_duplicated_index_names extends migration +class rename_duplicated_index_names extends container_aware_migration { public static function depends_on() { @@ -27,27 +27,25 @@ class rename_duplicated_index_names extends migration public function update_schema() { - $rename_index = $table_keys = []; - $db_table_schema = $this->get_schema(); - foreach ($db_table_schema as $table_name => $table_data) - { - if (isset($table_data['KEYS'])) - { - foreach ($table_data['KEYS'] as $key_name => $key_data) - { - $table_keys[$table_name][] = $key_name; - } - } - } + $rename_index = []; + $is_prefixed_index = false; + $tables_index_names = $this->get_tables_index_names(); + $short_table_names = table_helper::map_short_table_names(array_keys($tables_index_names), $this->table_prefix); - $short_table_names = table_helper::map_short_table_names([], $this->table_prefix); - foreach ($table_keys as $table_name => $key_names) + foreach ($tables_index_names as $table_name => $key_names) { foreach ($key_names as $key_name) { - $key_name_new = $short_table_names[$table_name] . '_' . $key_name; - $rename_index[$table_name][$key_name] = $key_name_new; - $rename_index[$table_name][$table_name . '_' . $key_name] = $key_name_new; + $prefixless_table_name = strpos($table_name, $this->table_prefix) === 0 ? substr($table_name, strlen($this->table_prefix)) : $table_name; + + // Check if there's at least one index name is prefixed, otherwise we operate on generated database schema + $is_prefixed_index = $is_prefixed_index || (strpos($key_name, $table_name) === 0); + + // If key name is prefixed by its table name (with or without tables prefix), remove that key name prefix. + $cleaned_key_name = !$is_prefixed_index ? $key_name : str_replace(strpos($key_name, $table_name) === 0 ? $table_name . '_' : $prefixless_table_name . '_', '', $key_name); + + $key_name_new = $short_table_names[$table_name] . '_' . $cleaned_key_name; + $rename_index[$table_name][$key_name !== $cleaned_key_name ? $key_name : $cleaned_key_name] = $key_name_new; } } @@ -88,4 +86,45 @@ class rename_duplicated_index_names extends migration return $schema_generator->get_schema(); } + + public function get_tables_index_names() + { + $table_keys = []; + $doctrine = $this->container->get('dbal.conn.doctrine'); + $schema_manager = $doctrine->createSchemaManager(); + $table_names = $schema_manager->listTableNames(); + + if (!empty($table_names)) + { + foreach ($table_names as $table_name) + { + $indices = $schema_manager->listTableIndexes($table_name); + + $index_names = array_keys( + array_filter($indices, function (\Doctrine\DBAL\Schema\Index $index) + { + return !$index->isPrimary(); + }) + ); + + if (!empty($index_names)) + { + $table_keys[$table_name] = $index_names; + } + } + } + else + { + $db_table_schema = $this->get_schema(); + foreach ($db_table_schema as $table_name => $table_data) + { + if (isset($table_data['KEYS'])) + { + $table_keys[$table_name] = array_keys($table_data['KEYS']); + } + } + } + + return $table_keys; + } } From 1cadb3818af3cf891722ba9bffc526d9ba098e6c Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 1 Jul 2025 14:27:38 +0700 Subject: [PATCH 14/22] [ticket/17525] Provide Doctrine connection object for migrations PHPBB-17525 --- .../migration/data/v400/rename_duplicated_index_names.php | 7 +++---- phpBB/phpbb/db/migration/migration.php | 6 ++++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index eb785878fb..b4c10271e6 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -13,10 +13,10 @@ namespace phpbb\db\migration\data\v400; -use phpbb\db\migration\container_aware_migration; +use phpbb\db\migration\migration; use phpbb\db\doctrine\table_helper; -class rename_duplicated_index_names extends container_aware_migration +class rename_duplicated_index_names extends migration { public static function depends_on() { @@ -90,8 +90,7 @@ class rename_duplicated_index_names extends container_aware_migration public function get_tables_index_names() { $table_keys = []; - $doctrine = $this->container->get('dbal.conn.doctrine'); - $schema_manager = $doctrine->createSchemaManager(); + $schema_manager = $this->db_doctrine->createSchemaManager(); $table_names = $schema_manager->listTableNames(); if (!empty($table_names)) diff --git a/phpBB/phpbb/db/migration/migration.php b/phpBB/phpbb/db/migration/migration.php index 6bfc395e4d..63290229f9 100644 --- a/phpBB/phpbb/db/migration/migration.php +++ b/phpBB/phpbb/db/migration/migration.php @@ -28,6 +28,9 @@ abstract class migration implements migration_interface /** @var \phpbb\db\driver\driver_interface */ protected $db; + /** @var \Doctrine\DBAL\Connection */ + protected $db_doctrine; + /** @var \phpbb\db\tools\tools_interface */ protected $db_tools; @@ -72,6 +75,9 @@ abstract class migration implements migration_interface $this->php_ext = $php_ext; $this->errors = array(); + + $phpbb_config_php_file = new \phpbb\config_php_file($phpbb_root_path, $php_ext); + $this->db_doctrine = \phpbb\db\doctrine\connection_factory::get_connection($phpbb_config_php_file); } /** From 4274faa7155a71cd9bbbdcfed39c2c337e45c217 Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 1 Jul 2025 15:53:19 +0700 Subject: [PATCH 15/22] [ticket/17525] Provide Doctrine connection via dbtools PHPBB-17525 --- .../migration/data/v400/rename_duplicated_index_names.php | 2 +- phpBB/phpbb/db/migration/migration.php | 6 ------ phpBB/phpbb/db/tools/doctrine.php | 8 ++++++++ 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index b4c10271e6..dc053a0f77 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -90,7 +90,7 @@ class rename_duplicated_index_names extends migration public function get_tables_index_names() { $table_keys = []; - $schema_manager = $this->db_doctrine->createSchemaManager(); + $schema_manager = $this->db_tools->get_connection()->createSchemaManager(); $table_names = $schema_manager->listTableNames(); if (!empty($table_names)) diff --git a/phpBB/phpbb/db/migration/migration.php b/phpBB/phpbb/db/migration/migration.php index 63290229f9..6bfc395e4d 100644 --- a/phpBB/phpbb/db/migration/migration.php +++ b/phpBB/phpbb/db/migration/migration.php @@ -28,9 +28,6 @@ abstract class migration implements migration_interface /** @var \phpbb\db\driver\driver_interface */ protected $db; - /** @var \Doctrine\DBAL\Connection */ - protected $db_doctrine; - /** @var \phpbb\db\tools\tools_interface */ protected $db_tools; @@ -75,9 +72,6 @@ abstract class migration implements migration_interface $this->php_ext = $php_ext; $this->errors = array(); - - $phpbb_config_php_file = new \phpbb\config_php_file($phpbb_root_path, $php_ext); - $this->db_doctrine = \phpbb\db\doctrine\connection_factory::get_connection($phpbb_config_php_file); } /** diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 73987d26ee..f74f251712 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -61,6 +61,14 @@ class doctrine implements tools_interface $this->connection = $connection; } + /** + * @return Connection + */ + public function get_connection(): Connection + { + return $this->connection; + } + /** * @return AbstractSchemaManager * From e9157f4d10f96f5d921ede7c87a101475b1e611f Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 1 Jul 2025 16:12:06 +0700 Subject: [PATCH 16/22] [ticket/17525] Fix tests PHPBB-17525 --- phpBB/phpbb/db/tools/doctrine.php | 2 +- phpBB/phpbb/db/tools/tools_interface.php | 7 +++++++ tests/dbal/migrator_test.php | 7 ++++--- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index f74f251712..a190e1f319 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -62,7 +62,7 @@ class doctrine implements tools_interface } /** - * @return Connection + * {@inheritDoc} */ public function get_connection(): Connection { diff --git a/phpBB/phpbb/db/tools/tools_interface.php b/phpBB/phpbb/db/tools/tools_interface.php index 09f47ed566..e8c23a5109 100644 --- a/phpBB/phpbb/db/tools/tools_interface.php +++ b/phpBB/phpbb/db/tools/tools_interface.php @@ -226,4 +226,11 @@ interface tools_interface * @return void */ public function sql_truncate_table(string $table_name): void; + + /** + * Gets current Doctrine DBAL connection + * + * @return \Doctrine\DBAL\Connection + */ + public function get_connection(): \Doctrine\DBAL\Connection; } diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index e7fbd4eb2d..4207e388de 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -485,9 +485,10 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertNotEmpty($table_keys); - $short_table_names = \phpbb\db\doctrine\table_helper::map_short_table_names(['custom_table' => 'cstmtbl'], 'phpbb_'); - $this->assertEquals('phpbb_custom_table', array_search('cstmtbl', $short_table_names)); - $this->assertEquals($short_table_names['phpbb_custom_table'], 'cstmtbl'); + $table_names = array_merge(array_keys($db_table_schema), ['phpbb_custom_table']); + $short_table_names = \phpbb\db\doctrine\table_helper::map_short_table_names($table_names, 'phpbb_'); + $this->assertEquals('phpbb_custom_table', array_search(\phpbb\db\doctrine\table_helper::generate_shortname('custom_table'), $short_table_names)); + $this->assertEquals($short_table_names['phpbb_custom_table'], \phpbb\db\doctrine\table_helper::generate_shortname('custom_table')); foreach ($table_keys as $table_name => $key_names) { From 8e0ec1edd27ce4f7830d540729fa33056ad0df2c Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 1 Jul 2025 21:01:09 +0700 Subject: [PATCH 17/22] [ticket/17525] Automatically handle index name prefixes PHPBB-17525 --- phpBB/phpbb/db/doctrine/table_helper.php | 2 +- phpBB/phpbb/db/tools/doctrine.php | 49 ++++++++++++++++++++++++ phpBB/phpbb/db/tools/tools_interface.php | 30 +++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index 150e98d95b..ae80e23a50 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -146,7 +146,7 @@ class table_helper /** * Generates short table names for the purpose of prefixing tables' index names. * - * @param string $table_name Table name with prefix to generate its short name. + * @param string $table_name Table name without prefix to generate its short name. * * @return string Short table name. */ diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index a190e1f319..2c52575651 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -168,6 +168,7 @@ class doctrine implements tools_interface */ public function sql_index_exists(string $table_name, string $index_name): bool { + $index_name = self::normalize_index_name($table_name, $index_name); return $this->asset_exists($index_name, $this->get_filtered_index_list($table_name, true)); } @@ -176,6 +177,7 @@ class doctrine implements tools_interface */ public function sql_unique_index_exists(string $table_name, string $index_name): bool { + $index_name = self::normalize_index_name($table_name, $index_name); return $this->asset_exists($index_name, $this->get_filtered_index_list($table_name, false)); } @@ -330,6 +332,7 @@ class doctrine implements tools_interface */ public function sql_create_index(string $table_name, string $index_name, $column) { + $index_name = self::normalize_index_name($table_name, $index_name); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name, $column): void { @@ -343,6 +346,8 @@ class doctrine implements tools_interface */ public function sql_rename_index(string $table_name, string $index_name_old, string $index_name_new) { + $index_name_old = self::normalize_index_name($table_name, $index_name_old); + $index_name_new = self::normalize_index_name($table_name, $index_name_new); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name_old, $index_name_new): void { @@ -356,6 +361,7 @@ class doctrine implements tools_interface */ public function sql_index_drop(string $table_name, string $index_name) { + $index_name = self::normalize_index_name($table_name, $index_name); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name): void { @@ -369,6 +375,7 @@ class doctrine implements tools_interface */ public function sql_create_unique_index(string $table_name, string $index_name, $column) { + $index_name = self::normalize_index_name($table_name, $index_name); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name, $column): void { @@ -405,6 +412,43 @@ class doctrine implements tools_interface } } + /** + * {@inheritDoc} + */ + public static function normalize_index_name(string $table_name, string $index_name, bool $remove_prefix = false): string + { + $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name)); + if (!self::is_prefixed($index_name, $short_table_name . '_')) + { + $index_name = $short_table_name . '_' . $index_name; + } + else if ($remove_prefix) + { + $index_name = remove_prefix($index_name); + } + + return $index_name; + } + + /** + * {@inheritDoc} + */ + public static function remove_prefix(string $name): string + { + $prefix_end_pos = strpos($name, '_'); + $prefixless_name = substr($name, $prefix_end_pos + 1); + + return $prefixless_name; + } + + /** + * {@inheritDoc} + */ + public static function is_prefixed(string $name, string $prefix): bool + { + return strpos($name, $prefix) === 0; + } + /** * Returns an array of indices for either unique and primary keys, or simple indices. * @@ -848,6 +892,7 @@ class doctrine implements tools_interface { $columns = (is_array($column)) ? $column : [$column]; $table = $schema->getTable($table_name); + $index_name = self::normalize_index_name($table_name, $index_name); if ($safe_check && $table->hasIndex($index_name)) { @@ -869,6 +914,8 @@ class doctrine implements tools_interface protected function schema_rename_index(Schema $schema, string $table_name, string $index_name_old, string $index_name_new, bool $safe_check = false): void { $table = $schema->getTable($table_name); + $index_name_old = self::normalize_index_name($table_name, $index_name_old); + $index_name_new = self::normalize_index_name($table_name, $index_name_new); if ($safe_check && !$table->hasIndex($index_name_old)) { @@ -891,6 +938,7 @@ class doctrine implements tools_interface { $columns = (is_array($column)) ? $column : [$column]; $table = $schema->getTable($table_name); + $index_name = self::normalize_index_name($table_name, $index_name); if ($safe_check && $table->hasIndex($index_name)) { @@ -911,6 +959,7 @@ class doctrine implements tools_interface protected function schema_index_drop(Schema $schema, string $table_name, string $index_name, bool $safe_check = false): void { $table = $schema->getTable($table_name); + $index_name = self::normalize_index_name($table_name, $index_name); if ($safe_check && !$table->hasIndex($index_name)) { diff --git a/phpBB/phpbb/db/tools/tools_interface.php b/phpBB/phpbb/db/tools/tools_interface.php index e8c23a5109..a3fdae8004 100644 --- a/phpBB/phpbb/db/tools/tools_interface.php +++ b/phpBB/phpbb/db/tools/tools_interface.php @@ -233,4 +233,34 @@ interface tools_interface * @return \Doctrine\DBAL\Connection */ public function get_connection(): \Doctrine\DBAL\Connection; + + /** + * Adds short table name prefix to the index name if needed + * + * @param string $table_name Table name with tables prefix + * @param string $index_name Index name + * @param bool $remove_prefix Flag indicating to remove short table name prefix if exists + * + * @return string Prefixed index name + */ + public static function normalize_index_name(string $table_name, string $index_name, bool $remove_prefix = false): string; + + /** + * Removes prefix from string if exists + * + * @param string $name String to remove the prefix from + * + * @return string Prefixless string + */ + public static function remove_prefix(string $name): string; + + /** + * Tests if a string is prefixed with the prefix defined + * + * @param string $name String to test vs prefix + * @param string $prefix Prefix name + * + * @return bool True if a string id prefixed with the prefix defined, false otherwise + */ + public static function is_prefixed(string $name, string $prefix): bool; } From 669af8f3aa308e4b8f6d001fc5fba629fb67fa1d Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 1 Jul 2025 23:32:36 +0700 Subject: [PATCH 18/22] [ticket/17525] Automatically handle index name prefixes PHPBB-17525 --- phpBB/phpbb/db/tools/doctrine.php | 3 ++- tests/dbal/db_tools_test.php | 39 +++++++++++++++++-------------- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 2c52575651..b5497a81d6 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -424,7 +424,7 @@ class doctrine implements tools_interface } else if ($remove_prefix) { - $index_name = remove_prefix($index_name); + $index_name = self::remove_prefix($index_name); } return $index_name; @@ -697,6 +697,7 @@ class doctrine implements tools_interface foreach ($table_data['KEYS'] as $key_name => $key_data) { $columns = (is_array($key_data[1])) ? $key_data[1] : [$key_data[1]]; + $key_name = self::normalize_index_name($table_name, $key_name); // Supports key columns defined with there length $columns = array_map(function (string $column) diff --git a/tests/dbal/db_tools_test.php b/tests/dbal/db_tools_test.php index 73543ed97d..42ce4e924b 100644 --- a/tests/dbal/db_tools_test.php +++ b/tests/dbal/db_tools_test.php @@ -232,23 +232,25 @@ class phpbb_dbal_db_tools_test extends phpbb_database_test_case public function test_column_change_with_index() { + $short_table_name = \phpbb\db\doctrine\table_helper::generate_shortname('table_name'); + // Create column $this->assertFalse($this->tools->sql_column_exists('prefix_table_name', 'c_bug_12012')); $this->assertTrue($this->tools->sql_column_add('prefix_table_name', 'c_bug_12012', array('DECIMAL', 0))); $this->assertTrue($this->tools->sql_column_exists('prefix_table_name', 'c_bug_12012')); // Create index over the column - $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', 'i_bug_12012')); + $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_i_bug_12012')); $this->assertTrue($this->tools->sql_create_index('prefix_table_name', 'i_bug_12012', array('c_bug_12012', 'c_bool'))); - $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', 'i_bug_12012')); + $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_i_bug_12012')); // Change type from int to string $this->assertTrue($this->tools->sql_column_change('prefix_table_name', 'c_bug_12012', array('VCHAR:100', ''))); // Remove the index - $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', 'i_bug_12012')); + $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_i_bug_12012')); $this->assertTrue($this->tools->sql_index_drop('prefix_table_name', 'i_bug_12012')); - $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', 'i_bug_12012')); + $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_i_bug_12012')); // Remove the column $this->assertTrue($this->tools->sql_column_exists('prefix_table_name', 'c_bug_12012')); @@ -301,19 +303,21 @@ class phpbb_dbal_db_tools_test extends phpbb_database_test_case public function test_column_remove_with_index() { + $short_table_name = \phpbb\db\doctrine\table_helper::generate_shortname('table_name'); + // Create column $this->assertFalse($this->tools->sql_column_exists('prefix_table_name', 'c_bug_12012_2')); $this->assertTrue($this->tools->sql_column_add('prefix_table_name', 'c_bug_12012_2', array('UINT', 4))); $this->assertTrue($this->tools->sql_column_exists('prefix_table_name', 'c_bug_12012_2')); // Create index over the column - $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', 'bug_12012_2')); + $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_bug_12012_2')); $this->assertTrue($this->tools->sql_create_index('prefix_table_name', 'bug_12012_2', array('c_bug_12012_2', 'c_bool'))); - $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', 'bug_12012_2')); + $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_bug_12012_2')); - $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', 'bug_12012_3')); + $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_bug_12012_3')); $this->assertTrue($this->tools->sql_create_index('prefix_table_name', 'bug_12012_3', array('c_bug_12012_2'))); - $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', 'bug_12012_3')); + $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_bug_12012_3')); // Remove the column $this->assertTrue($this->tools->sql_column_exists('prefix_table_name', 'c_bug_12012_2')); @@ -443,24 +447,24 @@ class phpbb_dbal_db_tools_test extends phpbb_database_test_case public function test_index_exists() { - $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', 'i_simple')); + $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', \phpbb\db\doctrine\table_helper::generate_shortname('table_name') . '_i_simple')); } public function test_unique_index_exists() { - $this->assertTrue($this->tools->sql_unique_index_exists('prefix_table_name', 'i_uniq')); + $this->assertTrue($this->tools->sql_unique_index_exists('prefix_table_name', \phpbb\db\doctrine\table_helper::generate_shortname('table_name') . '_i_uniq')); } public function test_create_index_against_index_exists() { $this->tools->sql_create_index('prefix_table_name', 'fookey', array('c_timestamp', 'c_decimal')); - $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', 'fookey')); + $this->assertTrue($this->tools->sql_index_exists('prefix_table_name', \phpbb\db\doctrine\table_helper::generate_shortname('table_name') . '_fookey')); } public function test_create_unique_index_against_unique_index_exists() { $this->tools->sql_create_unique_index('prefix_table_name', 'i_uniq_ts_id', array('c_timestamp', 'c_id')); - $this->assertTrue($this->tools->sql_unique_index_exists('prefix_table_name', 'i_uniq_ts_id')); + $this->assertTrue($this->tools->sql_unique_index_exists('prefix_table_name', \phpbb\db\doctrine\table_helper::generate_shortname('table_name') . '_i_uniq_ts_id')); } public function test_create_int_default_null() @@ -493,27 +497,28 @@ class phpbb_dbal_db_tools_test extends phpbb_database_test_case $table_suffix = str_repeat('a', 25 - strlen($table_prefix)); $table_name = $table_prefix . $table_suffix; + $short_table_name = \phpbb\db\doctrine\table_helper::generate_shortname($table_suffix); $this->tools->sql_create_table($table_name, $this->table_data); // Index name and table suffix and table prefix have > maximum index length chars in total. // Index name and table suffix have <= maximum index length chars in total. $long_index_name = str_repeat('i', $max_index_length - strlen($table_suffix)); - $this->assertFalse($this->tools->sql_index_exists($table_name, $long_index_name)); + $this->assertFalse($this->tools->sql_index_exists($table_name, $short_table_name . '_' . $long_index_name)); $this->assertTrue($this->tools->sql_create_index($table_name, $long_index_name, array('c_timestamp'))); - $this->assertTrue($this->tools->sql_index_exists($table_name, $long_index_name)); + $this->assertTrue($this->tools->sql_index_exists($table_name, $short_table_name . '_' . $long_index_name)); // Index name and table suffix have > maximum index length chars in total. $very_long_index_name = str_repeat('i', $max_index_length); - $this->assertFalse($this->tools->sql_index_exists($table_name, $very_long_index_name)); + $this->assertFalse($this->tools->sql_index_exists($table_name, $short_table_name . '_' . $very_long_index_name)); $this->assertTrue($this->tools->sql_create_index($table_name, $very_long_index_name, array('c_timestamp'))); - $this->assertTrue($this->tools->sql_index_exists($table_name, $very_long_index_name)); + $this->assertTrue($this->tools->sql_index_exists($table_name, $short_table_name . '_' . $very_long_index_name)); $this->tools->sql_table_drop($table_name); // Index name has > maximum index length chars - that should not be possible. $too_long_index_name = str_repeat('i', $max_index_length + 1); - $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', $too_long_index_name)); + $this->assertFalse($this->tools->sql_index_exists('prefix_table_name', $short_table_name . '_' . $too_long_index_name)); $this->setExpectedTriggerError(E_USER_ERROR); // TODO: Do we want to keep this limitation, if yes reimplement the user check /* https://github.com/phpbb/phpbb/blob/aee5e373bca6cd20d44b99585d3b758276a2d7e6/phpBB/phpbb/db/tools/tools.php#L1488-L1517 */ $this->tools->sql_create_index('prefix_table_name', $too_long_index_name, array('c_timestamp')); From b5c3befa87dac47c12dc23228a9a35c4090e4fdc Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 2 Jul 2025 11:01:49 +0700 Subject: [PATCH 19/22] [ticket/17525] Fix handling index name prefix logic for renaming PHPBB-17525 --- phpBB/phpbb/db/tools/doctrine.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index b5497a81d6..3853eae9cb 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -417,8 +417,12 @@ class doctrine implements tools_interface */ public static function normalize_index_name(string $table_name, string $index_name, bool $remove_prefix = false): string { - $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name)); - if (!self::is_prefixed($index_name, $short_table_name . '_')) + $prefixless_table_name = self::remove_prefix($table_name); + $short_table_name = table_helper::generate_shortname($prefixless_table_name); + if (!self::is_prefixed($index_name, $short_table_name . '_') + && !self::is_prefixed($index_name, $prefixless_table_name . '_') + && !self::is_prefixed($index_name, $table_name . '_') + ) { $index_name = $short_table_name . '_' . $index_name; } @@ -915,7 +919,6 @@ class doctrine implements tools_interface protected function schema_rename_index(Schema $schema, string $table_name, string $index_name_old, string $index_name_new, bool $safe_check = false): void { $table = $schema->getTable($table_name); - $index_name_old = self::normalize_index_name($table_name, $index_name_old); $index_name_new = self::normalize_index_name($table_name, $index_name_new); if ($safe_check && !$table->hasIndex($index_name_old)) From 5eaabb1c39303d202f9c7f1af34700325d83e491 Mon Sep 17 00:00:00 2001 From: rxu Date: Fri, 4 Jul 2025 00:08:28 +0700 Subject: [PATCH 20/22] [ticket/17525] Fix handling index name prefix logic for renaming PHPBB-17525 --- phpBB/develop/create_schema_files.php | 1 + phpBB/phpbb/db/doctrine/table_helper.php | 3 +- .../v400/rename_duplicated_index_names.php | 58 ++++++++------- phpBB/phpbb/db/tools/doctrine.php | 70 ++++++++++--------- phpBB/phpbb/db/tools/tools_interface.php | 28 +++++--- phpBB/phpbb/install/helper/database.php | 1 + .../install_database/task/add_tables.php | 1 + .../task/create_schema_file.php | 1 + tests/captcha/qa_test.php | 6 +- tests/console/user/base.php | 3 +- tests/dbal/auto_increment_test.php | 3 + tests/dbal/db_tools_test.php | 3 + tests/dbal/migrator_test.php | 3 + tests/extension/manager_test.php | 3 +- tests/extension/metadata_manager_test.php | 3 +- .../migrations_check_config_added_test.php | 8 ++- tests/migrator/convert_timezones_test.php | 12 +++- .../migrator/get_callable_from_step_test.php | 4 +- tests/migrator/schema_generator_test.php | 8 ++- tests/notification/convert_test.php | 3 +- tests/search/native_test.php | 3 +- .../phpbb_database_test_case.php | 1 + ...phpbb_database_test_connection_manager.php | 6 +- .../phpbb_functional_test_case.php | 1 + 24 files changed, 149 insertions(+), 84 deletions(-) diff --git a/phpBB/develop/create_schema_files.php b/phpBB/develop/create_schema_files.php index e7c44a7f34..a8c3fd8647 100644 --- a/phpBB/develop/create_schema_files.php +++ b/phpBB/develop/create_schema_files.php @@ -58,6 +58,7 @@ $db_doctrine = $ref->newInstanceWithoutConstructor(); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($db_doctrine, true); +$db_tools->set_table_prefix($table_prefix); $tables_data = \Symfony\Component\Yaml\Yaml::parseFile($phpbb_root_path . '/config/default/container/tables.yml'); $tables = []; diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index ae80e23a50..9c638a2ea4 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -13,6 +13,7 @@ namespace phpbb\db\doctrine; use InvalidArgumentException; +use phpbb\db\tools\doctrine as doctrine_dbtools; class table_helper { @@ -137,7 +138,7 @@ class table_helper $short_table_names_map = []; foreach ($table_names as $table_name) { - $short_table_names_map[$table_name] = self::generate_shortname(str_replace($table_prefix, '', $table_name)); + $short_table_names_map[$table_name] = self::generate_shortname(doctrine_dbtools::remove_prefix($table_name, $table_prefix)); } return $short_table_names_map; diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index dc053a0f77..9249c10744 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -15,9 +15,15 @@ namespace phpbb\db\migration\data\v400; use phpbb\db\migration\migration; use phpbb\db\doctrine\table_helper; +use phpbb\db\tools\doctrine as doctrine_dbtools; class rename_duplicated_index_names extends migration { + /** + * @var array + */ + protected $table_keys = []; + public static function depends_on() { return [ @@ -28,24 +34,33 @@ class rename_duplicated_index_names extends migration public function update_schema() { $rename_index = []; - $is_prefixed_index = false; - $tables_index_names = $this->get_tables_index_names(); - $short_table_names = table_helper::map_short_table_names(array_keys($tables_index_names), $this->table_prefix); - - foreach ($tables_index_names as $table_name => $key_names) + if (empty($this->table_keys)) { + $this->get_tables_index_names(); + } + $short_table_names = table_helper::map_short_table_names(array_keys($this->table_keys), $this->table_prefix); + + foreach ($this->table_keys as $table_name => $key_names) + { + $prefixless_table_name = doctrine_dbtools::remove_prefix($table_name, $this->table_prefix); foreach ($key_names as $key_name) { - $prefixless_table_name = strpos($table_name, $this->table_prefix) === 0 ? substr($table_name, strlen($this->table_prefix)) : $table_name; - - // Check if there's at least one index name is prefixed, otherwise we operate on generated database schema - $is_prefixed_index = $is_prefixed_index || (strpos($key_name, $table_name) === 0); - - // If key name is prefixed by its table name (with or without tables prefix), remove that key name prefix. - $cleaned_key_name = !$is_prefixed_index ? $key_name : str_replace(strpos($key_name, $table_name) === 0 ? $table_name . '_' : $prefixless_table_name . '_', '', $key_name); + // If 'old' key name is already new format, do not rename it + if (doctrine_dbtools::is_prefixed($key_name, $short_table_names[$table_name])) + { + continue; + } + // If 'old' key name is prefixed by its table name with and/or without table name common prefix + // (f.e. 'phpbb_log_log_time'), remove it to prefix with the relevant table's short name + $cleaned_key_name = $key_name; + foreach ([$table_name, $prefixless_table_name] as $prefix) + { + $cleaned_key_name = doctrine_dbtools::remove_prefix($cleaned_key_name, $prefix); + } $key_name_new = $short_table_names[$table_name] . '_' . $cleaned_key_name; - $rename_index[$table_name][$key_name !== $cleaned_key_name ? $key_name : $cleaned_key_name] = $key_name_new; + + $rename_index[$table_name][$key_name] = $key_name_new; } } @@ -89,7 +104,6 @@ class rename_duplicated_index_names extends migration public function get_tables_index_names() { - $table_keys = []; $schema_manager = $this->db_tools->get_connection()->createSchemaManager(); $table_names = $schema_manager->listTableNames(); @@ -98,32 +112,28 @@ class rename_duplicated_index_names extends migration foreach ($table_names as $table_name) { $indices = $schema_manager->listTableIndexes($table_name); - - $index_names = array_keys( - array_filter($indices, function (\Doctrine\DBAL\Schema\Index $index) + $indices = array_keys(array_filter($indices, + function (\Doctrine\DBAL\Schema\Index $index) { return !$index->isPrimary(); }) ); - if (!empty($index_names)) + if (!empty($indices)) { - $table_keys[$table_name] = $index_names; + $this->table_keys[$table_name] = $indices; } } } else { - $db_table_schema = $this->get_schema(); - foreach ($db_table_schema as $table_name => $table_data) + foreach ($this->get_schema() as $table_name => $table_data) { if (isset($table_data['KEYS'])) { - $table_keys[$table_name] = array_keys($table_data['KEYS']); + $this->table_keys[$table_name] = array_keys($table_data['KEYS']); } } } - - return $table_keys; } } diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 3853eae9cb..2fd72a5f38 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -49,6 +49,11 @@ class doctrine implements tools_interface */ private $return_statements; + /** + * @var string + */ + private $table_prefix; + /** * Database tools constructors. * @@ -94,6 +99,14 @@ class doctrine implements tools_interface return $this->get_schema_manager()->createSchema(); } + /** + * {@inheritDoc} + */ + public function set_table_prefix($table_prefix): void + { + $this->table_prefix = $table_prefix; + } + /** * {@inheritDoc} */ @@ -168,7 +181,6 @@ class doctrine implements tools_interface */ public function sql_index_exists(string $table_name, string $index_name): bool { - $index_name = self::normalize_index_name($table_name, $index_name); return $this->asset_exists($index_name, $this->get_filtered_index_list($table_name, true)); } @@ -177,7 +189,6 @@ class doctrine implements tools_interface */ public function sql_unique_index_exists(string $table_name, string $index_name): bool { - $index_name = self::normalize_index_name($table_name, $index_name); return $this->asset_exists($index_name, $this->get_filtered_index_list($table_name, false)); } @@ -332,7 +343,6 @@ class doctrine implements tools_interface */ public function sql_create_index(string $table_name, string $index_name, $column) { - $index_name = self::normalize_index_name($table_name, $index_name); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name, $column): void { @@ -346,8 +356,6 @@ class doctrine implements tools_interface */ public function sql_rename_index(string $table_name, string $index_name_old, string $index_name_new) { - $index_name_old = self::normalize_index_name($table_name, $index_name_old); - $index_name_new = self::normalize_index_name($table_name, $index_name_new); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name_old, $index_name_new): void { @@ -361,7 +369,6 @@ class doctrine implements tools_interface */ public function sql_index_drop(string $table_name, string $index_name) { - $index_name = self::normalize_index_name($table_name, $index_name); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name): void { @@ -375,7 +382,6 @@ class doctrine implements tools_interface */ public function sql_create_unique_index(string $table_name, string $index_name, $column) { - $index_name = self::normalize_index_name($table_name, $index_name); return $this->alter_schema( function (Schema $schema) use ($table_name, $index_name, $column): void { @@ -415,34 +421,18 @@ class doctrine implements tools_interface /** * {@inheritDoc} */ - public static function normalize_index_name(string $table_name, string $index_name, bool $remove_prefix = false): string + public static function add_prefix(string $name, string $prefix): string { - $prefixless_table_name = self::remove_prefix($table_name); - $short_table_name = table_helper::generate_shortname($prefixless_table_name); - if (!self::is_prefixed($index_name, $short_table_name . '_') - && !self::is_prefixed($index_name, $prefixless_table_name . '_') - && !self::is_prefixed($index_name, $table_name . '_') - ) - { - $index_name = $short_table_name . '_' . $index_name; - } - else if ($remove_prefix) - { - $index_name = self::remove_prefix($index_name); - } - - return $index_name; + return strpos($prefix, '_', -1) ? $prefix . $name : $prefix . '_' . $name; } /** * {@inheritDoc} */ - public static function remove_prefix(string $name): string + public static function remove_prefix(string $name, string $prefix = ''): string { - $prefix_end_pos = strpos($name, '_'); - $prefixless_name = substr($name, $prefix_end_pos + 1); - - return $prefixless_name; + $prefix = strpos($prefix, '_', -1) ? $prefix : $prefix . '_'; + return $prefix && self::is_prefixed($name, $prefix) ? substr($name, strlen($prefix)) : $name; } /** @@ -676,6 +666,7 @@ class doctrine implements tools_interface } $table = $schema->createTable($table_name); + $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name, $this->table_prefix)); $dbms_name = $this->get_schema_manager()->getDatabasePlatform()->getName(); foreach ($table_data['COLUMNS'] as $column_name => $column_data) @@ -701,7 +692,7 @@ class doctrine implements tools_interface foreach ($table_data['KEYS'] as $key_name => $key_data) { $columns = (is_array($key_data[1])) ? $key_data[1] : [$key_data[1]]; - $key_name = self::normalize_index_name($table_name, $key_name); + $key_name = !self::is_prefixed($key_name, $short_table_name) ? self::add_prefix($key_name, $short_table_name) : $key_name; // Supports key columns defined with there length $columns = array_map(function (string $column) @@ -897,7 +888,8 @@ class doctrine implements tools_interface { $columns = (is_array($column)) ? $column : [$column]; $table = $schema->getTable($table_name); - $index_name = self::normalize_index_name($table_name, $index_name); + $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name, $this->table_prefix)); + $index_name = !self::is_prefixed($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : $index_name; if ($safe_check && $table->hasIndex($index_name)) { @@ -919,7 +911,13 @@ class doctrine implements tools_interface protected function schema_rename_index(Schema $schema, string $table_name, string $index_name_old, string $index_name_new, bool $safe_check = false): void { $table = $schema->getTable($table_name); - $index_name_new = self::normalize_index_name($table_name, $index_name_new); + $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name, $this->table_prefix)); + + if (!$table->hasIndex($index_name_old)) + { + $index_name_old = !self::is_prefixed($index_name_old, $short_table_name) ? self::add_prefix($index_name_old, $short_table_name) : self::remove_prefix($index_name_old, $short_table_name); + } + $index_name_new = !self::is_prefixed($index_name_new, $short_table_name) ? self::add_prefix($index_name_new, $short_table_name) : $index_name_new; if ($safe_check && !$table->hasIndex($index_name_old)) { @@ -942,7 +940,8 @@ class doctrine implements tools_interface { $columns = (is_array($column)) ? $column : [$column]; $table = $schema->getTable($table_name); - $index_name = self::normalize_index_name($table_name, $index_name); + $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name, $this->table_prefix)); + $index_name = !self::is_prefixed($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : $index_name; if ($safe_check && $table->hasIndex($index_name)) { @@ -963,7 +962,12 @@ class doctrine implements tools_interface protected function schema_index_drop(Schema $schema, string $table_name, string $index_name, bool $safe_check = false): void { $table = $schema->getTable($table_name); - $index_name = self::normalize_index_name($table_name, $index_name); + $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name, $this->table_prefix)); + + if (!$table->hasIndex($index_name)) + { + $index_name = !self::is_prefixed($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : self::remove_prefix($index_name, $short_table_name); + } if ($safe_check && !$table->hasIndex($index_name)) { diff --git a/phpBB/phpbb/db/tools/tools_interface.php b/phpBB/phpbb/db/tools/tools_interface.php index a3fdae8004..36472f12c5 100644 --- a/phpBB/phpbb/db/tools/tools_interface.php +++ b/phpBB/phpbb/db/tools/tools_interface.php @@ -235,24 +235,25 @@ interface tools_interface public function get_connection(): \Doctrine\DBAL\Connection; /** - * Adds short table name prefix to the index name if needed + * Adds prefix to a string if needed * - * @param string $table_name Table name with tables prefix - * @param string $index_name Index name - * @param bool $remove_prefix Flag indicating to remove short table name prefix if exists + * @param string $name Table name with tables prefix + * @param string $prefix Prefix to add * - * @return string Prefixed index name + * @return string Prefixed name */ - public static function normalize_index_name(string $table_name, string $index_name, bool $remove_prefix = false): string; + public static function add_prefix(string $name, string $prefix): string; /** - * Removes prefix from string if exists + * Removes prefix from string if exists. If prefix is empty, + * the first part of the string ending with underscore will be removed. * - * @param string $name String to remove the prefix from + * @param string $name String to remove the prefix from + * @param string $prefix Prefix to remove * * @return string Prefixless string */ - public static function remove_prefix(string $name): string; + public static function remove_prefix(string $name, string $prefix = ''): string; /** * Tests if a string is prefixed with the prefix defined @@ -263,4 +264,13 @@ interface tools_interface * @return bool True if a string id prefixed with the prefix defined, false otherwise */ public static function is_prefixed(string $name, string $prefix): bool; + + /** + * Sets table prefix + * + * @param string $table_prefix String to test vs prefix + * + * @return void + */ + public function set_table_prefix(string $table_prefix): void; } diff --git a/phpBB/phpbb/install/helper/database.php b/phpBB/phpbb/install/helper/database.php index ddf4a503b7..91fd9c7aa8 100644 --- a/phpBB/phpbb/install/helper/database.php +++ b/phpBB/phpbb/install/helper/database.php @@ -393,6 +393,7 @@ class database $doctrine_db = connection_factory::get_connection_from_params($dbms, $dbhost, $dbuser, $dbpass, $dbname, $dbport); $db_tools_factory = new \phpbb\db\tools\factory(); $db_tools = $db_tools_factory->get($doctrine_db); + $db_tools->set_table_prefix($table_prefix); $tables = $db_tools->sql_list_tables(); $tables = array_map('strtolower', $tables); $table_intersect = array_intersect($tables, $table_ary); diff --git a/phpBB/phpbb/install/module/install_database/task/add_tables.php b/phpBB/phpbb/install/module/install_database/task/add_tables.php index 0d7e81b9e8..dfe276b6c9 100644 --- a/phpBB/phpbb/install/module/install_database/task/add_tables.php +++ b/phpBB/phpbb/install/module/install_database/task/add_tables.php @@ -98,6 +98,7 @@ class add_tables extends task_base $this->schema_file_path = $phpbb_root_path . 'store/schema.json'; $this->table_prefix = $this->config->get('table_prefix'); $this->change_prefix = $this->config->get('change_table_prefix', true); + $this->db_tools->set_table_prefix($this->table_prefix); parent::__construct(true); } diff --git a/phpBB/phpbb/install/module/install_database/task/create_schema_file.php b/phpBB/phpbb/install/module/install_database/task/create_schema_file.php index e75bca65dc..20615c134a 100644 --- a/phpBB/phpbb/install/module/install_database/task/create_schema_file.php +++ b/phpBB/phpbb/install/module/install_database/task/create_schema_file.php @@ -145,6 +145,7 @@ class create_schema_file extends \phpbb\install\task_base $migrator_classes = $finder->core_path('phpbb/db/migration/data/')->get_classes(); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($this->db_doctrine, true); + $db_tools->set_table_prefix($table_prefix); $tables_data = \Symfony\Component\Yaml\Yaml::parseFile($this->phpbb_root_path . '/config/default/container/tables.yml'); $tables = []; foreach ($tables_data['parameters'] as $parameter => $table) diff --git a/tests/captcha/qa_test.php b/tests/captcha/qa_test.php index 1c61a7d10c..f3b34302bc 100644 --- a/tests/captcha/qa_test.php +++ b/tests/captcha/qa_test.php @@ -25,7 +25,7 @@ class phpbb_captcha_qa_test extends \phpbb_database_test_case protected function setUp(): void { - global $db, $request, $phpbb_container; + global $db, $request, $phpbb_container, $table_prefix; $db = $this->new_dbal(); $db_doctrine = $this->new_doctrine_dbal(); @@ -35,7 +35,9 @@ class phpbb_captcha_qa_test extends \phpbb_database_test_case $request = new \phpbb_mock_request(); $phpbb_container = new \phpbb_mock_container_builder(); $factory = new \phpbb\db\tools\factory(); - $phpbb_container->set('dbal.tools', $factory->get($db_doctrine)); + $db_tools = $factory->get($db_doctrine); + $db_tools->set_table_prefix($table_prefix); + $phpbb_container->set('dbal.tools', $db_tools); $this->qa = new \phpbb\captcha\plugins\qa('phpbb_captcha_questions', 'phpbb_captcha_answers', 'phpbb_qa_confirm'); } diff --git a/tests/console/user/base.php b/tests/console/user/base.php index 511cfff9a3..1707ac2edb 100644 --- a/tests/console/user/base.php +++ b/tests/console/user/base.php @@ -32,7 +32,7 @@ abstract class phpbb_console_user_base extends phpbb_database_test_case protected function setUp(): void { - global $auth, $db, $cache, $config, $user, $phpbb_dispatcher, $phpbb_container, $phpbb_root_path, $phpEx; + global $auth, $db, $cache, $config, $user, $phpbb_dispatcher, $phpbb_container, $phpbb_root_path, $phpEx, $table_prefix; $phpbb_dispatcher = new \phpbb\event\dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); @@ -141,6 +141,7 @@ abstract class phpbb_console_user_base extends phpbb_database_test_case $factory = new \phpbb\db\tools\factory(); $db_doctrine = $this->new_doctrine_dbal(); $db_tools = $factory->get($db_doctrine); + $db_tools->set_table_prefix($table_prefix); $migrator = new \phpbb\db\migrator( $phpbb_container, $config, diff --git a/tests/dbal/auto_increment_test.php b/tests/dbal/auto_increment_test.php index e238859afc..5d34cb49b8 100644 --- a/tests/dbal/auto_increment_test.php +++ b/tests/dbal/auto_increment_test.php @@ -26,12 +26,15 @@ class phpbb_dbal_auto_increment_test extends phpbb_database_test_case protected function setUp(): void { + global $table_prefix; + parent::setUp(); $this->db = $this->new_dbal(); $this->db_doctrine = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); $this->tools = $factory->get($this->db_doctrine); + $this->tools->set_table_prefix($table_prefix); $this->table_data = array( 'COLUMNS' => array( diff --git a/tests/dbal/db_tools_test.php b/tests/dbal/db_tools_test.php index 42ce4e924b..5bd0705daa 100644 --- a/tests/dbal/db_tools_test.php +++ b/tests/dbal/db_tools_test.php @@ -35,12 +35,15 @@ class phpbb_dbal_db_tools_test extends phpbb_database_test_case protected function setUp(): void { + parent::setUp(); + $table_prefix = 'prefix_'; $this->db = $this->new_dbal(); $this->doctrine_db = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); $this->tools = $factory->get($this->doctrine_db); + $this->tools->set_table_prefix($table_prefix); $this->table_data = array( 'COLUMNS' => array( diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 4207e388de..f4b80fc5e9 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -53,12 +53,15 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case protected function setUp(): void { + global $table_prefix; + parent::setUp(); $this->db = $this->new_dbal(); $this->doctrine_db = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); $this->db_tools = $factory->get($this->doctrine_db); + $this->db_tools->set_table_prefix($table_prefix); $this->config = new \phpbb\config\db($this->db, new phpbb_mock_cache, 'phpbb_config'); diff --git a/tests/extension/manager_test.php b/tests/extension/manager_test.php index 73ac5a51de..e659d37a51 100644 --- a/tests/extension/manager_test.php +++ b/tests/extension/manager_test.php @@ -154,6 +154,7 @@ class phpbb_extension_manager_test extends phpbb_database_test_case { $phpbb_root_path = __DIR__ . './../../phpBB/'; $php_ext = 'php'; + $table_prefix = 'phpbb_'; $config = new \phpbb\config\config(array('version' => PHPBB_VERSION)); $db = $this->new_dbal(); @@ -162,7 +163,7 @@ class phpbb_extension_manager_test extends phpbb_database_test_case $factory = new \phpbb\db\tools\factory(); $finder_factory = new \phpbb\finder\factory(null, false, $phpbb_root_path, $php_ext); $db_tools = $factory->get($db_doctrine); - $table_prefix = 'phpbb_'; + $db_tools->set_table_prefix($table_prefix); $container = new phpbb_mock_container_builder(); diff --git a/tests/extension/metadata_manager_test.php b/tests/extension/metadata_manager_test.php index 095efd3a69..b32b67c124 100644 --- a/tests/extension/metadata_manager_test.php +++ b/tests/extension/metadata_manager_test.php @@ -37,6 +37,7 @@ class phpbb_extension_metadata_manager_test extends phpbb_database_test_case { parent::setUp(); + $this->table_prefix = 'phpbb_'; $this->config = new \phpbb\config\config(array( 'version' => '3.1.0', )); @@ -45,13 +46,13 @@ class phpbb_extension_metadata_manager_test extends phpbb_database_test_case $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $factory = new \phpbb\db\tools\factory(); $this->db_tools = $factory->get($this->db_doctrine); + $this->db_tools->set_table_prefix($this->table_prefix); $finder_factory = $this->createMock('\phpbb\finder\factory'); $this->phpbb_root_path = __DIR__ . '/'; $this->phpEx = 'php'; $this->cache = new \phpbb\cache\service(new phpbb_mock_cache(), $this->config, $this->db, $phpbb_dispatcher, $this->phpbb_root_path, $this->phpEx); - $this->table_prefix = 'phpbb_'; $container = new phpbb_mock_container_builder(); $cache_path = $this->phpbb_root_path . 'cache/twig'; diff --git a/tests/migrations/migrations_check_config_added_test.php b/tests/migrations/migrations_check_config_added_test.php index 1472f9e370..5cde980cde 100644 --- a/tests/migrations/migrations_check_config_added_test.php +++ b/tests/migrations/migrations_check_config_added_test.php @@ -55,6 +55,10 @@ class migrations_check_config_added_test extends phpbb_test_case { global $phpbb_root_path, $phpEx; + $this->table_prefix = 'phpbb_'; + $this->phpbb_root_path = $phpbb_root_path; + $this->php_ext = $phpEx; + $this->config = new \phpbb\config\config([ 'search_type' => '\phpbb\search\fulltext_mysql', ]); @@ -63,9 +67,7 @@ class migrations_check_config_added_test extends phpbb_test_case $this->db_doctrine = $this->createMock(\Doctrine\DBAL\Connection::class); $factory = new \phpbb\db\tools\factory(); $this->db_tools = $factory->get($this->db_doctrine); - $this->table_prefix = 'phpbb_'; - $this->phpbb_root_path = $phpbb_root_path; - $this->php_ext = $phpEx; + $this->db_tools->set_table_prefix($this->table_prefix); $tools = [ new \phpbb\db\migration\tool\config($this->config), diff --git a/tests/migrator/convert_timezones_test.php b/tests/migrator/convert_timezones_test.php index 7bc434e33c..9b7e80101b 100644 --- a/tests/migrator/convert_timezones_test.php +++ b/tests/migrator/convert_timezones_test.php @@ -18,10 +18,13 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case public function getDataSet() { + global $table_prefix; + $this->db = $this->new_dbal(); $this->db_doctrine = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($this->db_doctrine); + $db_tools->set_table_prefix($table_prefix); // user_dst doesn't exist anymore, must re-add it to test this $db_tools->sql_column_add('phpbb_users', 'user_dst', array('BOOL', 1)); @@ -55,16 +58,18 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case { parent::setUp(); - global $phpbb_root_path, $phpEx; + global $phpbb_root_path, $phpEx, $table_prefix; $this->db = $this->new_dbal(); $this->db_doctrine = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); + $db_tools = $factory->get($this->db_doctrine); + $db_tools->set_table_prefix($table_prefix); $this->migration = new \phpbb\db\migration\data\v310\timezone( new \phpbb\config\config(array()), $this->db, - $factory->get($this->db_doctrine), + $db_tools, $phpbb_root_path, $phpEx, 'phpbb_', @@ -84,6 +89,8 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case public function test_convert() { + global $table_prefix; + $this->migration->update_timezones(0); $sql = 'SELECT user_id, user_timezone @@ -98,6 +105,7 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($this->db_doctrine); + $db_tools->set_table_prefix($table_prefix); // Remove the user_dst field again $db_tools->sql_column_remove('phpbb_users', 'user_dst'); diff --git a/tests/migrator/get_callable_from_step_test.php b/tests/migrator/get_callable_from_step_test.php index 1200568046..f8a92c742a 100644 --- a/tests/migrator/get_callable_from_step_test.php +++ b/tests/migrator/get_callable_from_step_test.php @@ -23,6 +23,8 @@ class get_callable_from_step_test extends phpbb_database_test_case $db = $this->new_dbal(); $db_doctrine = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); + $db_tools = $factory->get($db_doctrine); + $db_tools->set_table_prefix($table_prefix); $user = $this->getMockBuilder('\phpbb\user')->disableOriginalConstructor()->getMock(); $user->ip = '127.0.0.1'; $module_manager = new \phpbb\module\module_manager( @@ -38,7 +40,7 @@ class get_callable_from_step_test extends phpbb_database_test_case new phpbb_mock_container_builder(), new \phpbb\config\config(array()), $db, - $factory->get($db_doctrine), + $db_tools, 'phpbb_migrations', $phpbb_root_path, $php_ext, diff --git a/tests/migrator/schema_generator_test.php b/tests/migrator/schema_generator_test.php index 04c73f925e..943029304f 100644 --- a/tests/migrator/schema_generator_test.php +++ b/tests/migrator/schema_generator_test.php @@ -30,14 +30,16 @@ class schema_generator_test extends phpbb_test_case parent::setUp(); + $this->table_prefix = 'phpbb_'; + $this->phpbb_root_path = $phpbb_root_path; + $this->php_ext = $phpEx; + $this->config = new \phpbb\config\config(array()); $this->db = new \phpbb\db\driver\sqlite3(); $this->doctrine_db = \phpbb\db\doctrine\connection_factory::get_connection(new phpbb_mock_config_php_file()); $factory = new \phpbb\db\tools\factory(); $this->db_tools = $factory->get($this->doctrine_db); - $this->table_prefix = 'phpbb_'; - $this->phpbb_root_path = $phpbb_root_path; - $this->php_ext = $phpEx; + $this->db_tools->set_table_prefix($this->table_prefix); } protected function get_schema_generator(array $class_names) diff --git a/tests/notification/convert_test.php b/tests/notification/convert_test.php index 6c1f7b496e..2a7de4ea84 100644 --- a/tests/notification/convert_test.php +++ b/tests/notification/convert_test.php @@ -25,12 +25,13 @@ class phpbb_notification_convert_test extends phpbb_database_test_case { parent::setUp(); - global $phpbb_root_path, $phpEx; + global $phpbb_root_path, $phpEx, $table_prefix; $this->db = $this->new_dbal(); $this->doctrine_db = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($this->doctrine_db); + $db_tools->set_table_prefix($table_prefix); $core_tables = self::get_core_tables(); // Add user_notify_type column for testing this migration and set type diff --git a/tests/search/native_test.php b/tests/search/native_test.php index 0d8d8e6810..edb0cc43fc 100644 --- a/tests/search/native_test.php +++ b/tests/search/native_test.php @@ -25,7 +25,7 @@ class phpbb_search_native_test extends phpbb_search_test_case protected function setUp(): void { - global $phpbb_root_path, $phpEx, $config, $cache; + global $phpbb_root_path, $phpEx, $config, $cache, $table_prefix; parent::setUp(); @@ -41,6 +41,7 @@ class phpbb_search_native_test extends phpbb_search_test_case $this->db = $this->new_dbal(); $tools_factory = new \phpbb\db\tools\factory(); $this->db_tools = $tools_factory->get($this->new_doctrine_dbal()); + $this->db_tools->set_table_prefix($table_prefix); $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $class = self::get_search_wrapper('\phpbb\search\backend\fulltext_native'); $config['fulltext_native_min_chars'] = 2; diff --git a/tests/test_framework/phpbb_database_test_case.php b/tests/test_framework/phpbb_database_test_case.php index 09de77bc35..76e77add44 100644 --- a/tests/test_framework/phpbb_database_test_case.php +++ b/tests/test_framework/phpbb_database_test_case.php @@ -88,6 +88,7 @@ abstract class phpbb_database_test_case extends TestCase $doctrine = \phpbb\db\doctrine\connection_factory::get_connection(new phpbb_mock_config_php_file()); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($doctrine, true); + $db_tools->set_table_prefix($table_prefix); $schema_generator = new \phpbb\db\migration\schema_generator($classes, new \phpbb\config\config(array()), $db, $db_tools, $phpbb_root_path, $phpEx, $table_prefix, self::get_core_tables()); file_put_contents(self::$schema_file, json_encode($schema_generator->get_schema())); diff --git a/tests/test_framework/phpbb_database_test_connection_manager.php b/tests/test_framework/phpbb_database_test_connection_manager.php index db73248f6c..8759a1f8c0 100644 --- a/tests/test_framework/phpbb_database_test_connection_manager.php +++ b/tests/test_framework/phpbb_database_test_connection_manager.php @@ -327,6 +327,8 @@ class phpbb_database_test_connection_manager */ protected function load_schema_from_file($directory, \phpbb\db\driver\driver_interface $db, \Doctrine\DBAL\Connection $doctrine) { + global $table_prefix; + $schema = $this->dbms['SCHEMA']; if ($this->config['dbms'] == 'phpbb\db\driver\mysql') @@ -363,7 +365,7 @@ class phpbb_database_test_connection_manager } else { - global $phpbb_root_path, $phpEx, $table_prefix; + global $phpbb_root_path, $phpEx; $finder = new \phpbb\finder\finder(null, false, $phpbb_root_path, $phpEx); $classes = $finder->core_path('phpbb/db/migration/data/') @@ -373,6 +375,7 @@ class phpbb_database_test_connection_manager $doctrine = \phpbb\db\doctrine\connection_factory::get_connection(new phpbb_mock_config_php_file()); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($doctrine, true); + $db_tools->set_table_prefix($table_prefix); $tables = phpbb_database_test_case::get_core_tables(); $schema_generator = new \phpbb\db\migration\schema_generator($classes, new \phpbb\config\config(array()), $db, $db_tools, $phpbb_root_path, $phpEx, $table_prefix, $tables); @@ -381,6 +384,7 @@ class phpbb_database_test_connection_manager $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($doctrine); + $db_tools->set_table_prefix($table_prefix); foreach ($db_table_schema as $table_name => $table_data) { $db_tools->sql_create_table( diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php index c42e0aa60b..d093768375 100644 --- a/tests/test_framework/phpbb_functional_test_case.php +++ b/tests/test_framework/phpbb_functional_test_case.php @@ -286,6 +286,7 @@ class phpbb_functional_test_case extends phpbb_test_case $factory = new \phpbb\db\tools\factory(); $finder_factory = new \phpbb\finder\factory(null, false, $phpbb_root_path, $phpEx); $db_tools = $factory->get($db_doctrine); + $db_tools->set_table_prefix(self::$config['table_prefix']); $container = new phpbb_mock_container_builder(); $migrator = new \phpbb\db\migrator( From 10921ebc582a10fdedb6dcd066695bdf3ec6eefc Mon Sep 17 00:00:00 2001 From: rxu Date: Fri, 4 Jul 2025 11:30:46 +0700 Subject: [PATCH 21/22] [ticket/17525] Fix migration failure on update PHPBB-17525 --- .../v400/rename_duplicated_index_names.php | 61 +++++++++++-------- phpBB/phpbb/db/migration/migration.php | 2 + 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index 9249c10744..1ee2f8b4d2 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -22,7 +22,12 @@ class rename_duplicated_index_names extends migration /** * @var array */ - protected $table_keys = []; + protected static $table_keys; + + /** + * @var array + */ + protected static $rename_index; public static function depends_on() { @@ -33,39 +38,41 @@ class rename_duplicated_index_names extends migration public function update_schema() { - $rename_index = []; - if (empty($this->table_keys)) + if (!isset(self::$rename_index)) { - $this->get_tables_index_names(); - } - $short_table_names = table_helper::map_short_table_names(array_keys($this->table_keys), $this->table_prefix); - - foreach ($this->table_keys as $table_name => $key_names) - { - $prefixless_table_name = doctrine_dbtools::remove_prefix($table_name, $this->table_prefix); - foreach ($key_names as $key_name) + if (!isset(self::$table_keys)) { - // If 'old' key name is already new format, do not rename it - if (doctrine_dbtools::is_prefixed($key_name, $short_table_names[$table_name])) - { - continue; - } + $this->get_tables_index_names(); + } + $short_table_names = table_helper::map_short_table_names(array_keys(self::$table_keys), $this->table_prefix); - // If 'old' key name is prefixed by its table name with and/or without table name common prefix - // (f.e. 'phpbb_log_log_time'), remove it to prefix with the relevant table's short name - $cleaned_key_name = $key_name; - foreach ([$table_name, $prefixless_table_name] as $prefix) + foreach (self::$table_keys as $table_name => $key_names) + { + $prefixless_table_name = doctrine_dbtools::remove_prefix($table_name, $this->table_prefix); + foreach ($key_names as $key_name) { - $cleaned_key_name = doctrine_dbtools::remove_prefix($cleaned_key_name, $prefix); - } - $key_name_new = $short_table_names[$table_name] . '_' . $cleaned_key_name; + // If 'old' key name is already new format, do not rename it + if (doctrine_dbtools::is_prefixed($key_name, $short_table_names[$table_name])) + { + continue; + } - $rename_index[$table_name][$key_name] = $key_name_new; + // If 'old' key name is prefixed by its table name with and/or without table name common prefix + // (f.e. 'phpbb_log_log_time'), remove it to prefix with the relevant table's short name + $cleaned_key_name = $key_name; + foreach ([$table_name, $prefixless_table_name] as $prefix) + { + $cleaned_key_name = doctrine_dbtools::remove_prefix($cleaned_key_name, $prefix); + } + $key_name_new = $short_table_names[$table_name] . '_' . $cleaned_key_name; + + self::$rename_index[$table_name][$key_name] = $key_name_new; + } } } return [ - 'rename_index' => $rename_index, + 'rename_index' => self::$rename_index, ]; } @@ -121,7 +128,7 @@ class rename_duplicated_index_names extends migration if (!empty($indices)) { - $this->table_keys[$table_name] = $indices; + self::$table_keys[$table_name] = $indices; } } } @@ -131,7 +138,7 @@ class rename_duplicated_index_names extends migration { if (isset($table_data['KEYS'])) { - $this->table_keys[$table_name] = array_keys($table_data['KEYS']); + self::$table_keys[$table_name] = array_keys($table_data['KEYS']); } } } diff --git a/phpBB/phpbb/db/migration/migration.php b/phpBB/phpbb/db/migration/migration.php index 6bfc395e4d..e0ea38ff80 100644 --- a/phpBB/phpbb/db/migration/migration.php +++ b/phpBB/phpbb/db/migration/migration.php @@ -72,6 +72,8 @@ abstract class migration implements migration_interface $this->php_ext = $php_ext; $this->errors = array(); + + $this->db_tools->set_table_prefix($this->table_prefix); } /** From aa3f266b8c71832f5821746bfbfe156147d46a93 Mon Sep 17 00:00:00 2001 From: rxu Date: Sat, 5 Jul 2025 12:27:10 +0700 Subject: [PATCH 22/22] [ticket/17525] Address the PR review comments PHPBB-17525 --- phpBB/adm/style/acp_ext_delete_data.html | 1 + phpBB/adm/style/acp_ext_enable.html | 1 + phpBB/includes/acp/acp_extensions.php | 10 ++-- phpBB/phpbb/db/doctrine/table_helper.php | 2 +- .../v400/rename_duplicated_index_names.php | 2 +- phpBB/phpbb/db/tools/doctrine.php | 46 ++++++++++++------- phpBB/phpbb/db/tools/tools_interface.php | 10 ---- tests/captcha/qa_test.php | 6 +-- tests/console/user/base.php | 3 +- tests/dbal/auto_increment_test.php | 3 +- tests/migrator/convert_timezones_test.php | 12 +---- tests/notification/convert_test.php | 3 +- tests/search/native_test.php | 3 +- 13 files changed, 48 insertions(+), 54 deletions(-) diff --git a/phpBB/adm/style/acp_ext_delete_data.html b/phpBB/adm/style/acp_ext_delete_data.html index 2d21506473..96ae816231 100644 --- a/phpBB/adm/style/acp_ext_delete_data.html +++ b/phpBB/adm/style/acp_ext_delete_data.html @@ -11,6 +11,7 @@

{{ lang('MIGRATION_EXCEPTION_ERROR') }}

{{ MIGRATOR_ERROR }}

+ {% if MIGRATOR_ERROR_STACK_TRACE %}

{{ MIGRATOR_ERROR_STACK_TRACE|nl2br }}

{% endif %}

{{ lang('RETURN_TO_EXTENSION_LIST') }}

{% elseif S_PRE_STEP %} diff --git a/phpBB/adm/style/acp_ext_enable.html b/phpBB/adm/style/acp_ext_enable.html index 5319b76d3a..4b2d71cb4c 100644 --- a/phpBB/adm/style/acp_ext_enable.html +++ b/phpBB/adm/style/acp_ext_enable.html @@ -11,6 +11,7 @@

{{ lang('MIGRATION_EXCEPTION_ERROR') }}

{{ MIGRATOR_ERROR }}

+ {% if MIGRATOR_ERROR_STACK_TRACE %}

{{ MIGRATOR_ERROR_STACK_TRACE|nl2br }}

{% endif %}

{{ lang('RETURN_TO_EXTENSION_LIST') }}

{% elseif S_PRE_STEP %} diff --git a/phpBB/includes/acp/acp_extensions.php b/phpBB/includes/acp/acp_extensions.php index 626f1b09c1..d5adf62936 100644 --- a/phpBB/includes/acp/acp_extensions.php +++ b/phpBB/includes/acp/acp_extensions.php @@ -278,9 +278,8 @@ class acp_extensions } catch (\Exception $e) { - $stack_trace = phpbb_filter_root_path(str_replace("\n", '
', $e->getTraceAsString())); - $message = $e->getMessage(); - $this->template->assign_var('MIGRATOR_ERROR', '' . $message . '

' . $stack_trace); + $this->template->assign_var('MIGRATOR_ERROR', $e->getMessage()); + $this->template->assign_var('MIGRATOR_ERROR_STACK_TRACE', phpbb_filter_root_path($e->getTraceAsString())); } $this->tpl_name = 'acp_ext_enable'; @@ -369,6 +368,11 @@ class acp_extensions { $this->template->assign_var('MIGRATOR_ERROR', $e->getLocalisedMessage($this->user)); } + catch (\Exception $e) + { + $this->template->assign_var('MIGRATOR_ERROR', $e->getMessage()); + $this->template->assign_var('MIGRATOR_ERROR_STACK_TRACE', phpbb_filter_root_path($e->getTraceAsString())); + } $this->tpl_name = 'acp_ext_delete_data'; diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index 9c638a2ea4..b163d75ccf 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -125,7 +125,7 @@ class table_helper } /** - * Maps table names to thair short names for the purpose of prefixing tables' index names. + * Maps table names to their short names for the purpose of prefixing tables' index names. * * @param array $table_names Table names with prefix to add to the map. * @param string $table_prefix Tables prefix. diff --git a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php index 1ee2f8b4d2..805d68ff75 100644 --- a/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php +++ b/phpBB/phpbb/db/migration/data/v400/rename_duplicated_index_names.php @@ -52,7 +52,7 @@ class rename_duplicated_index_names extends migration foreach ($key_names as $key_name) { // If 'old' key name is already new format, do not rename it - if (doctrine_dbtools::is_prefixed($key_name, $short_table_names[$table_name])) + if (str_starts_with($key_name, $short_table_names[$table_name])) { continue; } diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 2fd72a5f38..043252b4c5 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -423,7 +423,7 @@ class doctrine implements tools_interface */ public static function add_prefix(string $name, string $prefix): string { - return strpos($prefix, '_', -1) ? $prefix . $name : $prefix . '_' . $name; + return str_ends_with($prefix, '_') ? $prefix . $name : $prefix . '_' . $name; } /** @@ -431,16 +431,8 @@ class doctrine implements tools_interface */ public static function remove_prefix(string $name, string $prefix = ''): string { - $prefix = strpos($prefix, '_', -1) ? $prefix : $prefix . '_'; - return $prefix && self::is_prefixed($name, $prefix) ? substr($name, strlen($prefix)) : $name; - } - - /** - * {@inheritDoc} - */ - public static function is_prefixed(string $name, string $prefix): bool - { - return strpos($name, $prefix) === 0; + $prefix = str_ends_with($prefix, '_') ? $prefix : $prefix . '_'; + return $prefix && str_starts_with($name, $prefix) ? substr($name, strlen($prefix)) : $name; } /** @@ -692,7 +684,7 @@ class doctrine implements tools_interface foreach ($table_data['KEYS'] as $key_name => $key_data) { $columns = (is_array($key_data[1])) ? $key_data[1] : [$key_data[1]]; - $key_name = !self::is_prefixed($key_name, $short_table_name) ? self::add_prefix($key_name, $short_table_name) : $key_name; + $key_name = !str_starts_with($key_name, $short_table_name) ? self::add_prefix($key_name, $short_table_name) : $key_name; // Supports key columns defined with there length $columns = array_map(function (string $column) @@ -725,6 +717,8 @@ class doctrine implements tools_interface } /** + * Removes a table + * * @param Schema $schema * @param string $table_name * @param bool $safe_check @@ -742,6 +736,8 @@ class doctrine implements tools_interface } /** + * Adds column to a table + * * @param Schema $schema * @param string $table_name * @param string $column_name @@ -772,6 +768,8 @@ class doctrine implements tools_interface } /** + * Alters column properties + * * @param Schema $schema * @param string $table_name * @param string $column_name @@ -802,6 +800,8 @@ class doctrine implements tools_interface } /** + * Alters column properties or adds a column + * * @param Schema $schema * @param string $table_name * @param string $column_name @@ -824,6 +824,8 @@ class doctrine implements tools_interface } /** + * Removes a column in a table + * * @param Schema $schema * @param string $table_name * @param string $column_name @@ -876,6 +878,8 @@ class doctrine implements tools_interface } /** + * Creates non-unique index for a table + * * @param Schema $schema * @param string $table_name * @param string $index_name @@ -889,7 +893,7 @@ class doctrine implements tools_interface $columns = (is_array($column)) ? $column : [$column]; $table = $schema->getTable($table_name); $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name, $this->table_prefix)); - $index_name = !self::is_prefixed($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : $index_name; + $index_name = !str_starts_with($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : $index_name; if ($safe_check && $table->hasIndex($index_name)) { @@ -900,6 +904,8 @@ class doctrine implements tools_interface } /** + * Renames table index + * * @param Schema $schema * @param string $table_name * @param string $index_name_old @@ -915,9 +921,9 @@ class doctrine implements tools_interface if (!$table->hasIndex($index_name_old)) { - $index_name_old = !self::is_prefixed($index_name_old, $short_table_name) ? self::add_prefix($index_name_old, $short_table_name) : self::remove_prefix($index_name_old, $short_table_name); + $index_name_old = !str_starts_with($index_name_old, $short_table_name) ? self::add_prefix($index_name_old, $short_table_name) : self::remove_prefix($index_name_old, $short_table_name); } - $index_name_new = !self::is_prefixed($index_name_new, $short_table_name) ? self::add_prefix($index_name_new, $short_table_name) : $index_name_new; + $index_name_new = !str_starts_with($index_name_new, $short_table_name) ? self::add_prefix($index_name_new, $short_table_name) : $index_name_new; if ($safe_check && !$table->hasIndex($index_name_old)) { @@ -928,6 +934,8 @@ class doctrine implements tools_interface } /** + * Creates unique (non-primary) index for a table + * * @param Schema $schema * @param string $table_name * @param string $index_name @@ -941,7 +949,7 @@ class doctrine implements tools_interface $columns = (is_array($column)) ? $column : [$column]; $table = $schema->getTable($table_name); $short_table_name = table_helper::generate_shortname(self::remove_prefix($table_name, $this->table_prefix)); - $index_name = !self::is_prefixed($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : $index_name; + $index_name = !str_starts_with($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : $index_name; if ($safe_check && $table->hasIndex($index_name)) { @@ -952,6 +960,8 @@ class doctrine implements tools_interface } /** + * Removes table index + * * @param Schema $schema * @param string $table_name * @param string $index_name @@ -966,7 +976,7 @@ class doctrine implements tools_interface if (!$table->hasIndex($index_name)) { - $index_name = !self::is_prefixed($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : self::remove_prefix($index_name, $short_table_name); + $index_name = !str_starts_with($index_name, $short_table_name) ? self::add_prefix($index_name, $short_table_name) : self::remove_prefix($index_name, $short_table_name); } if ($safe_check && !$table->hasIndex($index_name)) @@ -978,6 +988,8 @@ class doctrine implements tools_interface } /** + * Creates primary key for a table + * * @param $column * @param Schema $schema * @param string $table_name diff --git a/phpBB/phpbb/db/tools/tools_interface.php b/phpBB/phpbb/db/tools/tools_interface.php index 36472f12c5..ceb1bb384d 100644 --- a/phpBB/phpbb/db/tools/tools_interface.php +++ b/phpBB/phpbb/db/tools/tools_interface.php @@ -255,16 +255,6 @@ interface tools_interface */ public static function remove_prefix(string $name, string $prefix = ''): string; - /** - * Tests if a string is prefixed with the prefix defined - * - * @param string $name String to test vs prefix - * @param string $prefix Prefix name - * - * @return bool True if a string id prefixed with the prefix defined, false otherwise - */ - public static function is_prefixed(string $name, string $prefix): bool; - /** * Sets table prefix * diff --git a/tests/captcha/qa_test.php b/tests/captcha/qa_test.php index f3b34302bc..1c61a7d10c 100644 --- a/tests/captcha/qa_test.php +++ b/tests/captcha/qa_test.php @@ -25,7 +25,7 @@ class phpbb_captcha_qa_test extends \phpbb_database_test_case protected function setUp(): void { - global $db, $request, $phpbb_container, $table_prefix; + global $db, $request, $phpbb_container; $db = $this->new_dbal(); $db_doctrine = $this->new_doctrine_dbal(); @@ -35,9 +35,7 @@ class phpbb_captcha_qa_test extends \phpbb_database_test_case $request = new \phpbb_mock_request(); $phpbb_container = new \phpbb_mock_container_builder(); $factory = new \phpbb\db\tools\factory(); - $db_tools = $factory->get($db_doctrine); - $db_tools->set_table_prefix($table_prefix); - $phpbb_container->set('dbal.tools', $db_tools); + $phpbb_container->set('dbal.tools', $factory->get($db_doctrine)); $this->qa = new \phpbb\captcha\plugins\qa('phpbb_captcha_questions', 'phpbb_captcha_answers', 'phpbb_qa_confirm'); } diff --git a/tests/console/user/base.php b/tests/console/user/base.php index 1707ac2edb..511cfff9a3 100644 --- a/tests/console/user/base.php +++ b/tests/console/user/base.php @@ -32,7 +32,7 @@ abstract class phpbb_console_user_base extends phpbb_database_test_case protected function setUp(): void { - global $auth, $db, $cache, $config, $user, $phpbb_dispatcher, $phpbb_container, $phpbb_root_path, $phpEx, $table_prefix; + global $auth, $db, $cache, $config, $user, $phpbb_dispatcher, $phpbb_container, $phpbb_root_path, $phpEx; $phpbb_dispatcher = new \phpbb\event\dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); @@ -141,7 +141,6 @@ abstract class phpbb_console_user_base extends phpbb_database_test_case $factory = new \phpbb\db\tools\factory(); $db_doctrine = $this->new_doctrine_dbal(); $db_tools = $factory->get($db_doctrine); - $db_tools->set_table_prefix($table_prefix); $migrator = new \phpbb\db\migrator( $phpbb_container, $config, diff --git a/tests/dbal/auto_increment_test.php b/tests/dbal/auto_increment_test.php index 5d34cb49b8..74c34c934f 100644 --- a/tests/dbal/auto_increment_test.php +++ b/tests/dbal/auto_increment_test.php @@ -26,10 +26,9 @@ class phpbb_dbal_auto_increment_test extends phpbb_database_test_case protected function setUp(): void { - global $table_prefix; - parent::setUp(); + $table_prefix = 'prefix_'; $this->db = $this->new_dbal(); $this->db_doctrine = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); diff --git a/tests/migrator/convert_timezones_test.php b/tests/migrator/convert_timezones_test.php index 9b7e80101b..7bc434e33c 100644 --- a/tests/migrator/convert_timezones_test.php +++ b/tests/migrator/convert_timezones_test.php @@ -18,13 +18,10 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case public function getDataSet() { - global $table_prefix; - $this->db = $this->new_dbal(); $this->db_doctrine = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($this->db_doctrine); - $db_tools->set_table_prefix($table_prefix); // user_dst doesn't exist anymore, must re-add it to test this $db_tools->sql_column_add('phpbb_users', 'user_dst', array('BOOL', 1)); @@ -58,18 +55,16 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case { parent::setUp(); - global $phpbb_root_path, $phpEx, $table_prefix; + global $phpbb_root_path, $phpEx; $this->db = $this->new_dbal(); $this->db_doctrine = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); - $db_tools = $factory->get($this->db_doctrine); - $db_tools->set_table_prefix($table_prefix); $this->migration = new \phpbb\db\migration\data\v310\timezone( new \phpbb\config\config(array()), $this->db, - $db_tools, + $factory->get($this->db_doctrine), $phpbb_root_path, $phpEx, 'phpbb_', @@ -89,8 +84,6 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case public function test_convert() { - global $table_prefix; - $this->migration->update_timezones(0); $sql = 'SELECT user_id, user_timezone @@ -105,7 +98,6 @@ class phpbb_migrator_convert_timezones_test extends phpbb_database_test_case $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($this->db_doctrine); - $db_tools->set_table_prefix($table_prefix); // Remove the user_dst field again $db_tools->sql_column_remove('phpbb_users', 'user_dst'); diff --git a/tests/notification/convert_test.php b/tests/notification/convert_test.php index 2a7de4ea84..6c1f7b496e 100644 --- a/tests/notification/convert_test.php +++ b/tests/notification/convert_test.php @@ -25,13 +25,12 @@ class phpbb_notification_convert_test extends phpbb_database_test_case { parent::setUp(); - global $phpbb_root_path, $phpEx, $table_prefix; + global $phpbb_root_path, $phpEx; $this->db = $this->new_dbal(); $this->doctrine_db = $this->new_doctrine_dbal(); $factory = new \phpbb\db\tools\factory(); $db_tools = $factory->get($this->doctrine_db); - $db_tools->set_table_prefix($table_prefix); $core_tables = self::get_core_tables(); // Add user_notify_type column for testing this migration and set type diff --git a/tests/search/native_test.php b/tests/search/native_test.php index edb0cc43fc..0d8d8e6810 100644 --- a/tests/search/native_test.php +++ b/tests/search/native_test.php @@ -25,7 +25,7 @@ class phpbb_search_native_test extends phpbb_search_test_case protected function setUp(): void { - global $phpbb_root_path, $phpEx, $config, $cache, $table_prefix; + global $phpbb_root_path, $phpEx, $config, $cache; parent::setUp(); @@ -41,7 +41,6 @@ class phpbb_search_native_test extends phpbb_search_test_case $this->db = $this->new_dbal(); $tools_factory = new \phpbb\db\tools\factory(); $this->db_tools = $tools_factory->get($this->new_doctrine_dbal()); - $this->db_tools->set_table_prefix($table_prefix); $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $class = self::get_search_wrapper('\phpbb\search\backend\fulltext_native'); $config['fulltext_native_min_chars'] = 2;