Fix SqlServerAdapter returning empty string instead of null for column default

[reeperbahnause]

The SqlServerAdapter::parseDefault($default) function returns "" if $default is null
Because of the returned empty string, the column can not be changed to have an empty string as default value (no differnce detected)

Example for an existing column which has no effect:

$this->table('tableName')->changeColumn('columnName', 'string', ['default' => '', 'null' => false])

[MasterOdin]

What version of SQLServer are you using, and how are you getting a column that has $default === null? Just want to understand where the current test I'd expect to catch this:

phinx/tests/Phinx/Db/Adapter/SqlServerAdapterTest.php

Lines 530 to 542 in 48d8e1f

	public function testChangeColumnDefaultToNull()
	{
	$table = new \Phinx\Db\Table('t', [], $this->adapter);
	$table->addColumn('column1', 'string', ['null' => true, 'default' => 'test'])
	->save();
	$newColumn1 = new \Phinx\Db\Table\Column();
	$newColumn1
	->setType('string')
	->setDefault(null);
	$table->changeColumn('column1', $newColumn1)->save();
	$columns = $this->adapter->getColumns('t');
	$this->assertNull($columns['column1']->getDefault());
	}

is failing as it should be that the assertNull on the last line would fail?

[reeperbahnause]

The SQL Server version is 15.0.2000

And the statement which was executed by phinx to get the current table structure was this (SqlServerAdapter::getColumns()):

SELECT DISTINCT TABLE_SCHEMA AS [schema], TABLE_NAME as [table_name], COLUMN_NAME AS [name], DATA_TYPE AS [type],
            IS_NULLABLE AS [null], COLUMN_DEFAULT AS [default],
            CHARACTER_MAXIMUM_LENGTH AS [char_length],
            NUMERIC_PRECISION AS [precision],
            NUMERIC_SCALE AS [scale], ORDINAL_POSITION AS [ordinal_position],
            COLUMNPROPERTY(object_id(TABLE_NAME), COLUMN_NAME, 'IsIdentity') as [identity]
        FROM INFORMATION_SCHEMA.COLUMNS
        WHERE TABLE_NAME = 'tableName'
        ORDER BY ordinal_position

[MasterOdin]

Sorry, when I said "how are you getting the column", I meant the phinx command that you were using to create the column, or is the column coming from somewhere else initially?

I'd like to make sure we have a test here for this code change, just not sure what to suggest for such a test and trying to gain a better understanding of the issue.

[reeperbahnause]

The tabe is initialy created like this:

$table = $this->table('users');
        $table->addColumn('login', 'string', array('limit' => 50))
            ->addColumn('firstname', 'string', array('limit' => 100))
            ->addColumn('lastname', 'string', array('limit' => 100))
            ->addColumn('status', 'string', array('limit' => 45))
            ->addColumn('hash', 'string', array('limit' => 255))
            ->addColumn('email', 'string', array('limit' => 255))
            ->addColumn('network', 'string', array('limit' => 255))
            ->addColumn('networkid', 'string', array('limit' => 255))
            ->addColumn('profileurl', 'string', array('limit' => 255))
            ->addColumn('imageurl', 'string', array('limit' => 255))
            ->addColumn('company', 'string', array('limit' => 255))
            ->addColumn('position', 'string', array('limit' => 255))
            ->addColumn('country', 'string', array('limit' => 255))
            ->addColumn('city', 'string', array('limit' => 255))
            ->addColumn('street', 'string', array('limit' => 255))
            ->addColumn('resettoken', 'string', array('limit' => 64, 'null' => true))
            ->create();

and the migration which did not have any effect was this:

$table = $this->table('users');
        $table->changeColumn('status', 'string', ['default' => '', 'null' => false])
            ->changeColumn('network', 'string', ['default' => '', 'null' => false])
            ->changeColumn('networkid', 'string', ['default' => '', 'null' => false])
            ->changeColumn('profileurl', 'string', ['default' => '', 'null' => false])
            ->changeColumn('imageurl', 'string', ['default' => '', 'null' => false])
            ->changeColumn('company', 'string', ['default' => '', 'null' => false])
            ->changeColumn('position', 'string', ['default' => '', 'null' => false])
            ->changeColumn('country', 'string', ['default' => '', 'null' => false])
            ->changeColumn('street', 'string', ['default' => '', 'null' => false])
            ->changeColumn('resettoken', 'string', ['default' => '', 'null'=>true])
            ->changeColumn('city', 'string', ['default' => '', 'null' => false])
        ->update();

Does that help?
If not let me know. I will get back in an hour.

[reeperbahnause]

Can I provide any more information regarding this issue?

[MasterOdin]

Sorry for the delay in getting to this @reeperbahnause! I've added a test case that verifies the change and made some slight tweaks, but the fix looks good. Seems like the bug was around that a NOT NULL column with no default would have a null value for column_default, while a NULL column with a null default would have "(NULL)" for column_default. I'm sure there's some important reason for this, but weird nevertheless.

This should be put out in a release soon.