SMF icon indicating copy to clipboard operation
SMF copied to clipboard

[2.1|3.0]: `change_column` automatically inserts old size

Open ThistleSifter opened this issue 8 months ago • 4 comments

Note: I haven't tested this in 3.0, but at a glance the relevant code looks the same.

When using $smcFunc['db_change_column'], if the old type has a size but the new type doesn't, the old type will be automatically appended.

For example, if changing from VARCHAR(255) to TEXT, the resulting query will have TEXT(255).

That one will execute, but changing from a TINYINT to an ENUM, like in the test script below, will result in a syntax error in the SQL.

Standalone test script for 2.1 (put in the same directory as SSI.php):

<?php

require_once(dirname(__FILE__) . '/SSI.php');

db_extend('packages');

$smcFunc['db_select_db']($db_name);

$smcFunc['db_create_table'](
	'test',
	[
		[
			'name' => 'id',
			'type' => 'int',
			'unsigned' => true,
			'null' => false,
			'auto' => true,
		],
		[
			'name' => 'some_value',
			'type' => 'tinyint',
			'unsigned' => true,
			'default' => 0,
			'null' => false,
		],
	],
	[
		[
			'type' => 'primary',
			'columns' => ['id'],
		]
	]
);

$smcFunc['db_change_column'](
	'test',
	'some_value',
	[
		'type' => 'ENUM(\'1\', \'2\', \'3\')',
		'null' => false,
		'drop_default' => true,
	]
);
// Results in:
// ALTER TABLE test
// CHANGE COLUMN `some_value` `some_value` enum('1', '2', '3')(3) NOT NULL   

ThistleSifter avatar Apr 13 '25 13:04 ThistleSifter

We should probably add some logic to check whether setting a size makes sense for the new type.

Sesquipedalian avatar Apr 13 '25 16:04 Sesquipedalian

It doesn't when changing from varchar to text. It might for some of the numeric data types though.

Oldiesmann avatar Apr 13 '25 20:04 Oldiesmann

The easiest solution is to require a size when changing the type that requires it. If you specify a type that differs from the current type, size must be specified if required by the type. Changing from varchar to text would not require it. Text to varchar would.

We should also no longer specify the "size" for int-based values. If the db schema requires it, we enforce the default. I mentioned int ones, because a decimal/float a size and precision is expected.

jdarwood007 avatar Apr 14 '25 02:04 jdarwood007

Currently, the function accepts a partial column definition and then attempts to fill in the gaps from the existing data. To simplify things, what if this was changed to require a full definition?

Then it would be up to the calling code to either write out a column definition in full or obtain one from list_columns and then modify or unset the values as needed.

ThistleSifter avatar Apr 14 '25 04:04 ThistleSifter