From ba0282bf38fff65205c23e60a2d2055b9b58913d Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 10 Jun 2025 10:16:02 +0700 Subject: [PATCH 1/5] [ticket/17524] Add possibility to use index key length in migrations PHPBB-17524 --- phpBB/phpbb/db/tools/doctrine.php | 51 ++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index bd3f71f83a..73d47117ce 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -629,25 +629,16 @@ 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]]; - - // Supports key columns defined with there length - $columns = array_map(function (string $column) - { - if (strpos($column, ':') !== false) - { - $parts = explode(':', $column, 2); - return $parts[0]; - } - return $column; - }, $columns); + $options = []; + $this->schema_get_index_key_data($columns, $options); if ($key_data[0] === 'UNIQUE') { - $table->addUniqueIndex($columns, $key_name); + $table->addUniqueIndex($columns, $key_name, $options); } else { - $table->addIndex($columns, $key_name); + $table->addIndex($columns, $key_name, [], $options); } } } @@ -830,7 +821,10 @@ class doctrine implements tools_interface return; } - $table->addIndex($columns, $index_name); + $options = []; + $this->schema_get_index_key_data($columns, $options); + + $table->addIndex($columns, $index_name, [], $options); } /** @@ -852,7 +846,10 @@ class doctrine implements tools_interface return; } - $table->addUniqueIndex($columns, $index_name); + $options = []; + $this->schema_get_index_key_data($columns, $options); + + $table->addUniqueIndex($columns, $index_name, $options); } /** @@ -891,6 +888,30 @@ class doctrine implements tools_interface $table->setPrimaryKey($columns); } + /** + * Checks if index data contains key length + * and put it into $options['lengths'] array. + * Handles key length in formats of 'keyname:123' or 'keyname(123)' + * + * @param array $columns + * @param array $options + */ + protected function schema_get_index_key_data(array &$columns, array &$options): void + { + if (!empty($columns)) + { + $columns = array_map(function (string $column) use (&$options) + { + if (preg_match('/^([a-zA-Z0-9_]+)(?:(?:\:|\()([0-9]{1,3})\)?)?$/', $column, $parts)) + { + $options['lengths'][] = $parts[2] ?? null; + return $parts[1]; + } + return $column; + }, $columns); + } + } + /** * Recreate an index of a table * From af93b1aee997502bae38b2590ecfac5a31d45006 Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 10 Jun 2025 11:31:48 +0700 Subject: [PATCH 2/5] [ticket/17524] Add index data getter to db tools PHPBB-17524 --- phpBB/phpbb/db/tools/doctrine.php | 44 +++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 73d47117ce..c7616a2a27 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -378,6 +378,46 @@ class doctrine implements tools_interface } } + /** + * Returns an array of the table index names and relevant data in format + * [ + * [$index_name] = [ + * 'columns' => (array) $index_columns, + * 'flags' => (array) $index_flags, + * 'options' => (array) $index_options, + * 'is_primary'=> (bool) $isPrimary, + * 'is_unique' => (bool) $isUnique, + * 'is_simple' => (bool) $isSimple, + * ] + * + * @param Schema $schema + * @param string $table_name + * @param string $column_name + * @param array $column_data + * @param bool $safe_check + * + * @throws SchemaException + */ + public function sql_get_table_index_data(string $table_name): array + { + $schema = $this->get_schema(); + $table = $schema->getTable($table_name); + $indexes = []; + foreach ($table->getIndexes() as $index) + { + $indexes[$index->getName()] = [ + 'columns' => array_map('strtolower', $index->getUnquotedColumns()), + 'flags' => $index->getFlags(), + 'options' => $index->getOptions(), + 'is_primary'=> $index->isPrimary(), + 'is_unique' => $index->isUnique(), + 'is_simple' => $index->isSimpleIndex(), + ]; + } + + return $indexes; + } + /** * Returns an array of indices for either unique and primary keys, or simple indices. * @@ -908,8 +948,8 @@ class doctrine implements tools_interface return $parts[1]; } return $column; - }, $columns); - } + }, $columns); + } } /** From 51c997aa22cd6037264a6938014c0f72c75fe32c Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 10 Jun 2025 13:55:48 +0700 Subject: [PATCH 3/5] [ticket/17524] Add index migrator tests PHPBB-17524 --- phpBB/phpbb/db/tools/doctrine.php | 6 +--- tests/dbal/migration/schema.php | 20 +++++++++---- tests/dbal/migrator_test.php | 48 +++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index c7616a2a27..9cc8ed3f6f 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -390,13 +390,9 @@ class doctrine implements tools_interface * 'is_simple' => (bool) $isSimple, * ] * - * @param Schema $schema * @param string $table_name - * @param string $column_name - * @param array $column_data - * @param bool $safe_check * - * @throws SchemaException + * @return array */ public function sql_get_table_index_data(string $table_name): array { diff --git a/tests/dbal/migration/schema.php b/tests/dbal/migration/schema.php index 38663bcc16..d91ed6a316 100644 --- a/tests/dbal/migration/schema.php +++ b/tests/dbal/migration/schema.php @@ -22,12 +22,22 @@ class phpbb_dbal_migration_schema extends \phpbb\db\migration\migration ), ), 'add_tables' => array( - $this->table_prefix . 'foobar' => array( - 'COLUMNS' => array( - 'module_id' => array('UINT:3', NULL, 'auto_increment'), - ), + $this->table_prefix . 'foobar' => [ + 'COLUMNS' => [ + 'module_id' => ['UINT:3', NULL, 'auto_increment'], + 'user_id' => ['ULINT', 0], + 'endpoint' => ['TEXT', ''], + 'expiration_time' => ['TIMESTAMP', 0], + 'p256dh' => ['VCHAR', ''], + 'auth' => ['VCHAR:100', ''], + ], 'PRIMARY_KEY' => 'module_id', - ), + 'KEYS' => [ + 'i_simple' => ['INDEX', ['user_id', 'endpoint:191']], + 'i_uniq' => ['UNIQUE', ['expiration_time', 'p256dh(100)']], + 'i_auth' => ['INDEX', 'auth'], + ], + ], ), ); } diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 5d16fa63ee..c497233557 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -408,6 +408,54 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertTrue($this->db_tools->sql_column_exists('phpbb_config', 'test_column1')); $this->assertTrue($this->db_tools->sql_table_exists('phpbb_foobar')); + $index_data_row = $this->db_tools->sql_get_table_index_data('phpbb_foobar'); + $is_mysql = $this->db->get_sql_layer() === 'mysqli'; // Index length only applies to MySQL indexes + $is_mssql = in_array($this->db->get_sql_layer(), ['mssqlnative', 'mssql_odbc']); // MSSQL primary index key has 'clustered' flag + foreach ($index_data_row as $index_name => $index_data) + { + switch ($index_name) + { + case 'i_simple': + $this->assertEquals(['user_id', 'endpoint'], $index_data['columns']); + $this->assertEmpty($index_data['flags']); + $this->assertFalse($index_data['is_primary']); + $this->assertFalse($index_data['is_unique']); + $this->assertTrue($index_data['is_simple']); + $this->assertEquals(2, count($index_data['options']['lengths'])); + $this->assertEmpty($index_data['options']['lengths'][0]); + $this->assertEquals($is_mysql ? 191 : null, $index_data['options']['lengths'][1]); + break; + case 'i_uniq': + $this->assertEquals(['expiration_time', 'p256dh'], $index_data['columns']); + $this->assertEmpty($index_data['flags']); + $this->assertFalse($index_data['is_primary']); + $this->assertTrue($index_data['is_unique']); + $this->assertFalse($index_data['is_simple']); + $this->assertEquals(2, count($index_data['options']['lengths'])); + $this->assertEmpty($index_data['options']['lengths'][0]); + $this->assertEquals($is_mysql ? 100 : null, $index_data['options']['lengths'][1]); + break; + case 'i_auth': + $this->assertEquals(['auth'], $index_data['columns']); + $this->assertEmpty($index_data['flags']); + $this->assertFalse($index_data['is_primary']); + $this->assertFalse($index_data['is_unique']); + $this->assertTrue($index_data['is_simple']); + $this->assertEquals(1, count($index_data['options']['lengths'])); + $this->assertEmpty($index_data['options']['lengths'][0]); + break; + default: // Primary key + $this->assertEquals(['module_id'], $index_data['columns']); + $this->assertEquals($is_mssql ? ['clustered'] : [], $index_data['flags']); + $this->assertTrue($index_data['is_primary']); + $this->assertTrue($index_data['is_unique']); + $this->assertFalse($index_data['is_simple']); + $this->assertEquals(1, count($index_data['options']['lengths'])); + $this->assertEmpty($index_data['options']['lengths'][0]); + break; + } + } + while ($this->migrator->migration_state('phpbb_dbal_migration_schema')) { $this->migrator->revert('phpbb_dbal_migration_schema'); From c7e94d4632a162c527a7bdab1245ba714493dbab Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 11 Jun 2025 21:04:56 +0700 Subject: [PATCH 4/5] [ticket/17524] Add test assertions PHPBB-17524 --- tests/dbal/migrator_test.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index c497233557..95bec1a133 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -409,7 +409,12 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertTrue($this->db_tools->sql_table_exists('phpbb_foobar')); $index_data_row = $this->db_tools->sql_get_table_index_data('phpbb_foobar'); - $is_mysql = $this->db->get_sql_layer() === 'mysqli'; // Index length only applies to MySQL indexes + $this->assertEquals(4, count($index_data_row)); + $this->assertTrue(isset($index_data_row['i_simple'])); + $this->assertTrue(isset($index_data_row['i_uniq'])); + $this->assertTrue(isset($index_data_row['i_auth'])); + + $is_mysql = $this->db->get_sql_layer() === 'mysqli'; // Key 'lengths' option only applies to MySQL indexes $is_mssql = in_array($this->db->get_sql_layer(), ['mssqlnative', 'mssql_odbc']); // MSSQL primary index key has 'clustered' flag foreach ($index_data_row as $index_name => $index_data) { From 3f4be7e3af83d7ed05a72d10cfd85e9fc267770f Mon Sep 17 00:00:00 2001 From: rxu Date: Thu, 12 Jun 2025 01:16:25 +0700 Subject: [PATCH 5/5] [ticket/17524] Try not to hit MSSQL index key length limitations 900 bytes for a clustered index. 1,700 bytes for a nonclustered index. For SQL Server 2014 (12.x) and earlier, all versions supported 900 bytes for all index types. PHPBB-17524 --- tests/dbal/migration/schema.php | 4 ++-- tests/dbal/migrator_test.php | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/dbal/migration/schema.php b/tests/dbal/migration/schema.php index d91ed6a316..04c20c46cb 100644 --- a/tests/dbal/migration/schema.php +++ b/tests/dbal/migration/schema.php @@ -26,9 +26,9 @@ class phpbb_dbal_migration_schema extends \phpbb\db\migration\migration 'COLUMNS' => [ 'module_id' => ['UINT:3', NULL, 'auto_increment'], 'user_id' => ['ULINT', 0], - 'endpoint' => ['TEXT', ''], + 'endpoint' => ['VCHAR:220', ''], 'expiration_time' => ['TIMESTAMP', 0], - 'p256dh' => ['VCHAR', ''], + 'p256dh' => ['VCHAR:200', ''], 'auth' => ['VCHAR:100', ''], ], 'PRIMARY_KEY' => 'module_id', diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 95bec1a133..0350427dd9 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -415,14 +415,16 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertTrue(isset($index_data_row['i_auth'])); $is_mysql = $this->db->get_sql_layer() === 'mysqli'; // Key 'lengths' option only applies to MySQL indexes - $is_mssql = in_array($this->db->get_sql_layer(), ['mssqlnative', 'mssql_odbc']); // MSSQL primary index key has 'clustered' flag + // MSSQL primary index key has 'clustered' flag, 'nonclustered' otherwise + // See https://learn.microsoft.com/en-us/sql/relational-databases/indexes/clustered-and-nonclustered-indexes-described?view=sql-server-ver17#indexes-and-constraints + $is_mssql = in_array($this->db->get_sql_layer(), ['mssqlnative', 'mssql_odbc']); foreach ($index_data_row as $index_name => $index_data) { switch ($index_name) { case 'i_simple': $this->assertEquals(['user_id', 'endpoint'], $index_data['columns']); - $this->assertEmpty($index_data['flags']); + $this->assertEquals($is_mssql ? ['nonclustered'] : [], $index_data['flags']); $this->assertFalse($index_data['is_primary']); $this->assertFalse($index_data['is_unique']); $this->assertTrue($index_data['is_simple']); @@ -432,7 +434,7 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case break; case 'i_uniq': $this->assertEquals(['expiration_time', 'p256dh'], $index_data['columns']); - $this->assertEmpty($index_data['flags']); + $this->assertEquals($is_mssql ? ['nonclustered'] : [], $index_data['flags']); $this->assertFalse($index_data['is_primary']); $this->assertTrue($index_data['is_unique']); $this->assertFalse($index_data['is_simple']); @@ -442,7 +444,7 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case break; case 'i_auth': $this->assertEquals(['auth'], $index_data['columns']); - $this->assertEmpty($index_data['flags']); + $this->assertEquals($is_mssql ? ['nonclustered'] : [], $index_data['flags']); $this->assertFalse($index_data['is_primary']); $this->assertFalse($index_data['is_unique']); $this->assertTrue($index_data['is_simple']);