diff --git a/lib/ddl/mssql_sql_generator.php b/lib/ddl/mssql_sql_generator.php index c03a69ad4f7..1fcd98a77b0 100644 --- a/lib/ddl/mssql_sql_generator.php +++ b/lib/ddl/mssql_sql_generator.php @@ -283,8 +283,16 @@ class mssql_sql_generator extends sql_generator { return array(); } - // Call to standard (parent) getRenameFieldSQL() function - $results = array_merge($results, parent::getRenameFieldSQL($xmldb_table, $xmldb_field, $newname)); + // We can't call to standard (parent) getRenameFieldSQL() function since it would enclose the field name + // with improper quotes in MSSQL: here, we use a stored procedure to rename the field i.e. a column object; + // we need to take care about how this stored procedure expects parameters to be "qualified". + $rename = str_replace('TABLENAME', $this->getTableName($xmldb_table), $this->rename_column_sql); + // Qualifying the column object could require brackets: use them, regardless the column name not being a reserved word. + $rename = str_replace('OLDFIELDNAME', '[' . $xmldb_field->getName() . ']', $rename); + // The new field name should be passed as the actual name, w/o any quote. + $rename = str_replace('NEWFIELDNAME', $newname, $rename); + + $results[] = $rename; return $results; } diff --git a/lib/ddl/mysql_sql_generator.php b/lib/ddl/mysql_sql_generator.php index e9f44185787..d768ca521e3 100644 --- a/lib/ddl/mysql_sql_generator.php +++ b/lib/ddl/mysql_sql_generator.php @@ -510,7 +510,7 @@ class mysql_sql_generator extends sql_generator { $fieldsql = $this->getFieldSQL($xmldb_table, $xmldb_field_clone); $sql = 'ALTER TABLE ' . $this->getTableName($xmldb_table) . ' CHANGE ' . - $xmldb_field->getName() . ' ' . $fieldsql; + $this->getEncQuoted($xmldb_field->getName()) . ' ' . $fieldsql; return array($sql); } diff --git a/lib/ddl/tests/ddl_test.php b/lib/ddl/tests/ddl_test.php index 2f42a6276e1..76e1d011f15 100644 --- a/lib/ddl/tests/ddl_test.php +++ b/lib/ddl/tests/ddl_test.php @@ -1545,12 +1545,45 @@ class core_ddl_testcase extends database_driver_testcase { $field = new xmldb_field('type'); $field->set_attributes(XMLDB_TYPE_CHAR, '20', null, XMLDB_NOTNULL, null, 'general', 'course'); + // 1. Rename the 'type' field into a generic new valid name. + // This represents the standard use case. $dbman->rename_field($table, $field, 'newfieldname'); $columns = $DB->get_columns('test_table0'); $this->assertArrayNotHasKey('type', $columns); $this->assertArrayHasKey('newfieldname', $columns); + $field->setName('newfieldname'); + + // 2. Rename the 'newfieldname' field into a reserved word, for testing purposes. + // This represents a questionable use case: we should support it but discourage the use of it on peer reviewing. + $dbman->rename_field($table, $field, 'where'); + + $columns = $DB->get_columns('test_table0'); + + $this->assertArrayNotHasKey('newfieldname', $columns); + $this->assertArrayHasKey('where', $columns); + + // 3. Create a table with a column name named w/ a reserved word and get rid of it. + // This represents a "recovering" use case: a field name could be a reserved word in the future, at least for a DB type. + $table = new xmldb_table('test_table_res_word'); + $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $table->add_field('where', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0'); + $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); + $table->setComment("This is a test'n drop table. You can drop it safely"); + $dbman->create_table($table); + $dbman->table_exists('test_table_res_word'); + + $columns = $DB->get_columns('test_table_res_word'); + $this->assertArrayHasKey('where', $columns); + $field = $table->getField('where'); + + $dbman->rename_field($table, $field, 'newfieldname'); + + $columns = $DB->get_columns('test_table_res_word'); + + $this->assertArrayNotHasKey('where', $columns); + $this->assertArrayHasKey('newfieldname', $columns); } public function testIndexExists() { @@ -2168,7 +2201,7 @@ class core_ddl_testcase extends database_driver_testcase { * This is a test for sql_generator::getEncQuoted(). * * @dataProvider test_get_enc_quoted_provider - * @param string $reserved Whether the column name is reserved or not. + * @param bool $reserved Whether the column name is reserved or not. * @param string $columnname The column name to be quoted, according to the value of $reserved. **/ public function test_get_enc_quoted($reserved, $columnname) { @@ -2197,6 +2230,98 @@ class core_ddl_testcase extends database_driver_testcase { } } + /** + * Data provider for test_sql_generator_get_rename_field_sql(). + * + * @return array The type-old-new tuple fixture. + */ + public function test_sql_generator_get_rename_field_sql_provider() { + return array( + // Reserved: an example from SQL-92. + // Both names should be reserved. + [true, 'from', 'where'], + // Not reserved. + [false, 'my_old_column_name', 'my_awesome_column_name'] + ); + } + + /** + * This is a unit test for sql_generator::getRenameFieldSQL(). + * + * @dataProvider test_sql_generator_get_rename_field_sql_provider + * @param bool $reserved Whether the column name is reserved or not. + * @param string $oldcolumnname The column name to be renamed. + * @param string $newcolumnname The new column name. + **/ + public function test_sql_generator_get_rename_field_sql($reserved, $oldcolumnname, $newcolumnname) { + $DB = $this->tdb; + $gen = $DB->get_manager()->generator; + $prefix = $DB->get_prefix(); + + $tablename = 'test_get_rename_field_sql'; + $table = new xmldb_table($tablename); + $field = new xmldb_field($oldcolumnname, XMLDB_TYPE_INTEGER, '11', null, XMLDB_NOTNULL, null, null, null, '0', 'previous'); + + $dbfamily = $DB->get_dbfamily(); + if (!$reserved) { + // No need to quote the column name. + switch ($dbfamily) { + case 'mysql': + $this->assertSame( + [ "ALTER TABLE {$prefix}$tablename CHANGE $oldcolumnname $newcolumnname BIGINT(11) NOT NULL" ], + $gen->getRenameFieldSQL($table, $field, $newcolumnname) + ); + break; + case 'sqlite': + // Skip it, since the DB is not supported yet. + // BTW renaming a column name is already covered by the integration test 'testRenameField'. + break; + case 'mssql': // The Moodle connection runs under 'QUOTED_IDENTIFIER ON'. + $this->assertSame( + [ "sp_rename '{$prefix}$tablename.[$oldcolumnname]', '$newcolumnname', 'COLUMN'" ], + $gen->getRenameFieldSQL($table, $field, $newcolumnname) + ); + break; + case 'oracle': + case 'postgres': + default: + $this->assertSame( + [ "ALTER TABLE {$prefix}$tablename RENAME COLUMN $oldcolumnname TO $newcolumnname" ], + $gen->getRenameFieldSQL($table, $field, $newcolumnname) + ); + break; + } + } else { + // Column name should be quoted. + switch ($dbfamily) { + case 'mysql': + $this->assertSame( + [ "ALTER TABLE {$prefix}$tablename CHANGE `$oldcolumnname` `$newcolumnname` BIGINT(11) NOT NULL" ], + $gen->getRenameFieldSQL($table, $field, $newcolumnname) + ); + break; + case 'sqlite': + // Skip it, since the DB is not supported yet. + // BTW renaming a column name is already covered by the integration test 'testRenameField'. + break; + case 'mssql': // The Moodle connection runs under 'QUOTED_IDENTIFIER ON'. + $this->assertSame( + [ "sp_rename '{$prefix}$tablename.[$oldcolumnname]', '$newcolumnname', 'COLUMN'" ], + $gen->getRenameFieldSQL($table, $field, $newcolumnname) + ); + break; + case 'oracle': + case 'postgres': + default: + $this->assertSame( + [ "ALTER TABLE {$prefix}$tablename RENAME COLUMN \"$oldcolumnname\" TO \"$newcolumnname\"" ], + $gen->getRenameFieldSQL($table, $field, $newcolumnname) + ); + break; + } + } + } + // Following methods are not supported == Do not test. /* public function testRenameIndex() {