fix modified column being nullable

[LordSimal]

Since we are now more tightly integrating the migrations plugin with the CakePHP framework I'd propose to change the default updated/modified column type to not be nullable, as by the TimestampBehavior this will always be set.

[LordSimal]

@markstory is this maybe a bug in the base schema dialect?

It didn't correctly detect the on update CURRENT_TIMESTAMP as it seems - but just for MySQL

[LordSimal]

found the issue - the Extra column in that describe query contains

DEFAULT_GENERATED on update CURRENT_TIMESTAMP

as you can see here

  [1] =>
  array(9) {
    'Field' =>
    string(7) "created"
    'Type' =>
    string(8) "datetime"
    'Collation' =>
    NULL
    'Null' =>
    string(2) "NO"
    'Key' =>
    string(0) ""
    'Default' =>
    string(17) "CURRENT_TIMESTAMP"
    'Extra' =>
    string(17) "DEFAULT_GENERATED"
    'Privileges' =>
    string(31) "select,insert,update,references"
    'Comment' =>
    string(0) ""
  }
  [2] =>
  array(9) {
    'Field' =>
    string(7) "updated"
    'Type' =>
    string(8) "datetime"
    'Collation' =>
    NULL
    'Null' =>
    string(2) "NO"
    'Key' =>
    string(0) ""
    'Default' =>
    string(17) "CURRENT_TIMESTAMP"
    'Extra' =>
    string(45) "DEFAULT_GENERATED on update CURRENT_TIMESTAMP"
    'Privileges' =>
    string(31) "select,insert,update,references"
    'Comment' =>
    string(0) ""
  }

mysql> SHOW FULL COLUMNS FROM table1;
+---------+--------------+-----------+------+-----+-------------------+-----------------------------------------------+---------------------------------+---------+
| Field   | Type         | Collation | Null | Key | Default           | Extra                                         | Privileges                      | Comment |
+---------+--------------+-----------+------+-----+-------------------+-----------------------------------------------+---------------------------------+---------+
| id      | int unsigned | NULL      | NO   | PRI | NULL              | auto_increment                                | select,insert,update,references |         |
| created | datetime     | NULL      | NO   |     | CURRENT_TIMESTAMP | DEFAULT_GENERATED                             | select,insert,update,references |         |
| updated | datetime     | NULL      | NO   |     | CURRENT_TIMESTAMP | DEFAULT_GENERATED on update CURRENT_TIMESTAMP | select,insert,update,references |         |
+---------+--------------+-----------+------+-----+-------------------+-----------------------------------------------+---------------------------------+---------+

[markstory]

Do we still need all of this with the change made to cake core?

[LordSimal]

Nope, will re-check this with latest 5.x

[LordSimal]

So the change in cakephp/cakephp#19207 now causes these tests to fail as the serialized schema dumps do not contain that new property.

@markstory @dereuromark I have no idea how those schema-dump files are generated

[markstory]

I have no idea how those schema-dump files are generated

I usually just update those by hand 😊

[LordSimal]

Thanks to Codex we now have a PHP script to automatically update all the Diff Schema Dump Files instead of having to manually adjust/update those serialized dump files whenever we decide/need to change something in our database class properties.

[LordSimal]

So should this be released in a patch release or should this rather go in a feature release?

[dereuromark]

5.next could be safer, not sure what the implications are for users:

• Release as a minor version (not patch)
• Mention in release notes that modified columns are now non-nullable

[dereuromark]

Concerns

1. This is a behavior change, not a bug fix. It changes how schemas are generated, which could cause:
   - Schema inconsistency between old and new migrations
   - Issues for anyone using addTimestamps() without TimestampBehavior
2. Edge case - What about apps that intentionally leave modified as null on first insert and only set it on update? (Rare, but possible.) With this change, even the initial insert would get a timestamp.

[LordSimal]

I have adjusted the base to be 6.x
This will go into a new major to let people more easily adjust to the new 5.x migrations major version first.

About schema inconsistencies - we already have that and your tools plugin helps with getting it in sync.
For apps intentionally leaving modified as null - they can just manually call the addColumn method with the desired options.

[markstory]

Seems reasonable to do in a major release.