Fix text column length change

[cbotsikas]

Q	A
Type	bug
BC Break	no
Fixed issues	#2566

Summary

Schema Comparator's diffColumn did not check if the length had changed when the column type was Text. In MySQL it wasn't possible to change between TINYTEXT, TEXT, MEDIUMTEXT and LONGTEXT.
I've added a new check for TextType where i only check if the length property is the same in both columns. I did not append the case with StringType and BinaryType because 'fixed' property doesn't apply to TextType and the default 'length' of 255 feels wrong.

I've included a test case.

[jwage]

After this change, the correct DDL statement is generated for you?

[cbotsikas]

I need to check this later today and also use the non strict comparison (!=) instead. I suppose this is why Travis failed and I guess that's why it's not strict for the StringType case.

[cbotsikas]

@jwage I've successfully tested it using Laravel with MySQL and all migrations i tried switching between text, mediumtext and longtext where executed correctly. The DDL was correct. Laravel doesn't support tinytext, so i didn't do that.

Regarding the travis failing test, I think it's out of my scope and not sure how to proceed. Either the failing test is wrong or there's a deeper issue with the field's length default handling in dbal. Let me explain the issue:
The test is creating a text field without specifying the length. This means that the object (and the $offlineTable) has null length for the column. This column is created as longtext in MySQL for some reason. When the test reads the table in $onlineTable, the length is 0 since there is no case for longtext as there exists for the rest (tinytext, text and mediumtext). My latest commit with non-strict comparison is checking 0 over null, which is making the test for MySQL to pass. For SqlServer, i'm not sure what's the actual type but the length is -1, which is the MAX length, if i'm not mistaken. Comparing -1 over null is not the same and shouldn't. Without my fix, all tests are passing but clearly it shouldn't since the length values are not equal.

What do you think about this issue and the failing test?

In my opinion, it should use the strict comparison (but could also stay with the non-strict one) and fix the failing test with explicit length or by checking manually if the diff is the text column's length and ignore it.

I don't have any more capacity atm to dive deeper in dbal.

[cbotsikas]

With MySQL, changing the test to explicitly state the text length has the following results:

• existing test with no length, Fail with strict comparison, Pass with non-strict:
  $table->addColumn('foo', 'text', array('notnull' => true));
  $offlineTable's column length: null
  $onlineTable's column length: 0
• tinytext. Pass:
  $table->addColumn('foo', 'text', array('length' => 255, 'notnull' => true));
  $offlineTable's column length: 255
  $onlineTable's column length: 255
• text, Pass:
  $table->addColumn('foo', 'text', array('length' => 65535, 'notnull' => true));
  $offlineTable's column length: 65535
  $onlineTable's column length: 65535
• mediumtext, Pass:
  $table->addColumn('foo', 'text', array('length' => 16777215, 'notnull' => true));
  $offlineTable's column length: 16777215
  $onlineTable's column length: 16777215
• longtext, Fail:
  $table->addColumn('foo', 'text', array('length' => 4294967295, 'notnull' => true));
  $offlineTable's column length: 4294967295
  $onlineTable's column length: 0
• random, Fail (expected):
  $table->addColumn('foo', 'text', array('length' => 50, 'notnull' => true));
  $offlineTable's column length: 50
  $onlineTable's column length: 255
• -1, Fail (expected):
  $table->addColumn('foo', 'text', array('length' => -1, 'notnull' => true));
  $offlineTable's column length: -1
  $onlineTable's column length: 255

The longtext shouldn't be failing but, as i said, the case is missing when the table is read.
I feel that the SqlServer would fail in any case except maybe the last, but i didn't check how is the SqlServer implementation done and what would it mean to set the length for text.

[PVince81]

Any chance to get the tests fixed to have this merged ?

Is there another workaround for the time being ?

[aschempp]

Any chance of this getting fixed?

@cbotsikas a suggestion to fix the failing test. Try this:

if (($properties1['length'] > 0 || $properties2['length'] > 0) && (int) $properties1['length'] !== (int) $properties2['length']) {
    $changedProperties[] = 'length';
}

[SenseException]

It seems the changes break sqlsrv and ibm_db2 tests. Any chances that you can make your changes work with them too?

[morozov]

Closing as stale. The issue should be addressed by #4659.