Fix SqlServerAdapter returning empty string instead of null for column default by reeperbahnause · Pull Request #2090 · cakephp/phinx

reeperbahnause · 2022-04-25T18:45:31Z

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])

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)

MasterOdin · 2022-04-25T19:00:21Z

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 · 2022-04-25T19:27:08Z

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 · 2022-04-25T19:29:27Z

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 · 2022-04-25T19:33:19Z

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 · 2022-05-04T09:26:24Z

Can I provide any more information regarding this issue?

Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>

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.

SqlServerAdapter::parseDefault

7354fbc

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)

dereuromark reviewed Jul 4, 2022

View reviewed changes

Comment thread src/Phinx/Db/Adapter/SqlServerAdapter.php

MasterOdin added 2 commits July 9, 2022 14:35

add testcase, fix docstring

593af76

Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>

phpcs

d70662f

Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>

MasterOdin changed the title ~~SqlServerAdapter::parseDefault~~ Fix SqlServerAdapter::parseDefault returning empty string instead of null Jul 9, 2022

MasterOdin changed the title ~~Fix SqlServerAdapter::parseDefault returning empty string instead of null~~ Fix SqlServerAdapter returning empty string instead of null for column default Jul 9, 2022

Update SqlServerAdapter.php

be7355e

MasterOdin approved these changes Jul 9, 2022

View reviewed changes

MasterOdin merged commit 9a6ce1e into cakephp:0.x Jul 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SqlServerAdapter returning empty string instead of null for column default#2090

Fix SqlServerAdapter returning empty string instead of null for column default#2090
MasterOdin merged 4 commits intocakephp:0.xfrom
reeperbahnause:patch-2

reeperbahnause commented Apr 25, 2022

Uh oh!

MasterOdin commented Apr 25, 2022 •

edited

Loading

Uh oh!

reeperbahnause commented Apr 25, 2022

Uh oh!

MasterOdin commented Apr 25, 2022

Uh oh!

reeperbahnause commented Apr 25, 2022

Uh oh!

reeperbahnause commented May 4, 2022

Uh oh!

Uh oh!

MasterOdin left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

reeperbahnause commented Apr 25, 2022

Uh oh!

MasterOdin commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reeperbahnause commented Apr 25, 2022

Uh oh!

MasterOdin commented Apr 25, 2022

Uh oh!

reeperbahnause commented Apr 25, 2022

Uh oh!

reeperbahnause commented May 4, 2022

Uh oh!

Uh oh!

MasterOdin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MasterOdin commented Apr 25, 2022 •

edited

Loading