From aef616381ec41a481b4970b77c62f2f2ef42ca39 Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 10 Jun 2025 10:16:02 +0700 Subject: [PATCH 1/6] [ticket/17524] Add possibility to use index key length in migrations PHPBB-17524 --- phpBB/phpbb/db/tools/doctrine.php | 50 ++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 043252b4c5..49ff2a23b6 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -686,24 +686,16 @@ class doctrine implements tools_interface $columns = (is_array($key_data[1])) ? $key_data[1] : [$key_data[1]]; $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) - { - 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); } } } @@ -900,7 +892,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); } /** @@ -956,7 +951,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); } /** @@ -1005,6 +1003,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 418aa469d79139c8725047eb5845a2fcc508d07c Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 10 Jun 2025 11:31:48 +0700 Subject: [PATCH 2/6] [ticket/17524] Add index data getter to db tools PHPBB-17524 --- phpBB/phpbb/db/tools/doctrine.php | 40 +++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 49ff2a23b6..93c25ad9ae 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -435,6 +435,42 @@ class doctrine implements tools_interface return $prefix && str_starts_with($name, $prefix) ? substr($name, strlen($prefix)) : $name; } + /** + * 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 string $table_name + * + * @return array + */ + 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. * @@ -1023,8 +1059,8 @@ class doctrine implements tools_interface return $parts[1]; } return $column; - }, $columns); - } + }, $columns); + } } /** From c47e5d34df840aa4c29241f5eb896a04b0cee225 Mon Sep 17 00:00:00 2001 From: rxu Date: Tue, 10 Jun 2025 13:55:48 +0700 Subject: [PATCH 3/6] [ticket/17524] Add index migrator tests PHPBB-17524 --- tests/dbal/migration/schema.php | 20 ++++++++++---- tests/dbal/migrator_test.php | 48 +++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 5 deletions(-) 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 f4b80fc5e9..2ef16282d3 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -412,6 +412,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 ffcd71ac87a504c387606a568efd7dbae37b0e36 Mon Sep 17 00:00:00 2001 From: rxu Date: Wed, 11 Jun 2025 21:04:56 +0700 Subject: [PATCH 4/6] [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 2ef16282d3..0fc827c7db 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -413,7 +413,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 967a7349dadfefd7fc1281da58c80ecdced3654f Mon Sep 17 00:00:00 2001 From: rxu Date: Thu, 12 Jun 2025 01:16:25 +0700 Subject: [PATCH 5/6] [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 0fc827c7db..fee4cd6699 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -419,14 +419,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']); @@ -436,7 +438,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']); @@ -446,7 +448,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']); From 868b5350f70faefeef886fe4ed698f03b86cce86 Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 6 Jul 2025 09:00:39 +0700 Subject: [PATCH 6/6] [ticket/17524] Fix tests PHPBB-17524 --- tests/dbal/migrator_test.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index fee4cd6699..b6541b8a11 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -57,11 +57,12 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case parent::setUp(); + $this->table_prefix = $table_prefix; $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->db_tools->set_table_prefix($this->table_prefix); $this->config = new \phpbb\config\db($this->db, new phpbb_mock_cache, 'phpbb_config'); @@ -412,21 +413,24 @@ 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')); + $short_table_name = \phpbb\db\doctrine\table_helper::generate_shortname('foobar'); $index_data_row = $this->db_tools->sql_get_table_index_data('phpbb_foobar'); $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'])); + $this->assertTrue(isset($index_data_row[$short_table_name . '_i_simple'])); + $this->assertTrue(isset($index_data_row[$short_table_name . '_i_uniq'])); + $this->assertTrue(isset($index_data_row[$short_table_name . '_i_auth'])); $is_mysql = $this->db->get_sql_layer() === 'mysqli'; // Key 'lengths' option only applies to MySQL indexes + // 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': + case $short_table_name . '_i_simple': $this->assertEquals(['user_id', 'endpoint'], $index_data['columns']); $this->assertEquals($is_mssql ? ['nonclustered'] : [], $index_data['flags']); $this->assertFalse($index_data['is_primary']); @@ -436,7 +440,7 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertEmpty($index_data['options']['lengths'][0]); $this->assertEquals($is_mysql ? 191 : null, $index_data['options']['lengths'][1]); break; - case 'i_uniq': + case $short_table_name . '_i_uniq': $this->assertEquals(['expiration_time', 'p256dh'], $index_data['columns']); $this->assertEquals($is_mssql ? ['nonclustered'] : [], $index_data['flags']); $this->assertFalse($index_data['is_primary']); @@ -446,7 +450,7 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->assertEmpty($index_data['options']['lengths'][0]); $this->assertEquals($is_mysql ? 100 : null, $index_data['options']['lengths'][1]); break; - case 'i_auth': + case $short_table_name . '_i_auth': $this->assertEquals(['auth'], $index_data['columns']); $this->assertEquals($is_mssql ? ['nonclustered'] : [], $index_data['flags']); $this->assertFalse($index_data['is_primary']);