Craft 2 DbHelper fault which for example errors plugin Migration field definitions

[narration-sd]

Description

A field added in the database wasn't turning out according to its string definition in the migration safeUp().

The reason turns out to be not considering string definitions throughout DbHelper. In this case, the block at line 223 improperly added a 'NULL' to the end of the original definition. This, compounded with MySql's 'interesting' defaulting, ended up with the confusing result in the database.

A look through Craft 3 source appears to show you aren't intercepting Yii 2's handling in the way DbHelper did, so potentially no problem of this kind, but you'll know accurately.

Steps to reproduce

1. attempt a migration like:

   $this->addColumn( 'craft_grnplatform_subscriptions ', 'failedRenew', "tinyint(1) unsigned not null default 0");

2. note via a tool that you don't get 'not null' in the resulting database field characteristics, though the migration reports run succeeded.

3. other interesting things can occur, or not occur, trying other string definition, similarly not what would be expected, because of the interaction with MySql defaulting patterns under their conditions.

4. note that defining this kind of $type/$config in array form does give correct database field results, as does a full sql text execute(). Though there is a caveat addressed in the comment below....

Additional info

• Craft version: 2.6.2993
• PHP version: 7.1.2-4+deb.sury.org~xenial+1
• Database driver & version: pdo_mysql
• Plugins & versions:

[narration-sd]

The show is next week, so no doubt this won't get early attention, but in case you're operating from email on this, want to note I've taken a suggestion out of the problem statement, as it wasn't quite right, and will put the corrected version here.

What goes wrong in the condition at line 223 is straightforward enough, but fixing it right there not as much, and a question would also be if there are other conditions in DbHelper::generateColumnDefinition that also don't consider that the original $ccnfig might have been a string.

Thinking about a solid solution turns up another issue to fix: the initial check for a PK is going go wrong if the $config passed is an array, isn't it?

Given you accept an early return for that case, it seems the test there could be expanded to return first if the passed config is a string, and then if the PK definition shows up in the array $config, now that we would know it's in that form. The string test part covers the original intent, if PK were passed that way.

Including comment update, sometihng like the following, if you find you agree:

// Don't do anything to string configs at all, or array PKs.
if (is_string($config) || $config['column'] === ColumnType::PK) {
    return $config;
}

Hope I may have it right to all intents this way -- and if so, interesting how the picture about this came up early the next morning, out of the blue. How the mind works, isn't it; sometimes I really think the best way it does.

[brandonkelly]

Yeah this is an area where Craft 1/2 didn’t follow Yii conventions, to a fault. You can work around it by changing your addColumn() call to:

$this->addColumn( 'craft_grnplatform_subscriptions ', 'failedRenew', [
    'column' => ColumnType::TinyInt,
    'unsigned' => true,
    'null' => false,
    'default' => 0
]);

(In Craft 3 you can just do this using normal Yii code.)

[narration-sd]

Yeah, this is what I suggested to the person who had the problem.

https://craftcms.stackexchange.com/a/22524/85

I thought the PK par was also a subtle way to have problems, but maybe you think not worth a quick fix because C2 is going away....

[narration-sd]

Or maybe Closed - Bug already means it goes on a list... :)

[brandonkelly]

Yeah I’d rather not change that code at this point – too risky it’ll end up breaking something else, and it’s already fixed for v3.