getEmptyValueByFieldType returns NULL value by mistake

[vstuber]

The function "getEmptyValueByFieldType" first checks whether the field's sql definition contains "NULL" but not "NOT NULL" and returns NULL if this is the case. If it's not, the function checks whether the field has one of the following types: 'binary', 'varbinary', 'tinyblob', 'blob', 'mediumblob', 'longblob'.

If it does, NULL is returned, even though the function never made sure that NULL is actually allowed for this field.

A field defined as "varbinary(128) NOT NULL default ''" causes the function to return NULL which results in a fatal error when this value is written to the database.

Some extensions still use field definitions like this, especially since for quite some time this was contao's definition for alias fields, which, of course, has also been used by extension developers.

Since Contao 3.5.14 (I think) this function is also used when copying records in the backend which means that copying db records with varbinary alias fields causes the system to crash.

[leofeyer]

@contao/developers /cc

[ausi]

I was able to reproduce it with:

$GLOBALS['TL_DCA']['tl_content']['fields']['test_field'] = array(
    'eval' => array('doNotCopy' => true),
    'sql' => "varbinary(128) NOT NULL default ''",
);

Which results in a “Query error: Column 'test_field' cannot be null” when trying to duplicate a content element.

I think we should remove Widget.php:1529-1532 to fix this issue.

[vstuber]

I think we should remove Widget.php:1529-1532 to fix this issue.

This way we wouldn't get NULL as the empty value for a varbinary field (or blob etc.) even if it was a valid value.

I suggest that, instead of

elseif (in_array($type, array('binary', 'varbinary', 'tinyblob', 'blob', 'mediumblob', 'longblob')))
{
    return null;
}

we use something like

elseif (in_array($type, array('binary', 'varbinary', 'tinyblob', 'blob', 'mediumblob', 'longblob')))
{
    return strpos($sql, 'NOT NULL') === false ? null : '';
}

[leofeyer]

Fixed in 073c1c3.

[fritzmg]

@vstuber no I think definitions like varbinary(128) NULL will be catched by

if (strpos($sql, 'NULL') !== false && strpos($sql, 'NOT NULL') === false)
{
    return null;
}

before that.

[ausi]

I think we can improve this further if we change

if (strpos($sql, 'NULL') !== false && strpos($sql, 'NOT NULL') === false)

to

if (strpos($sql, 'NOT NULL') === false)

because NULL is the default.

[leofeyer]

Then null would be returned for everything that does not contain NOT NULL wouldn't it?

`tstamp` int(10) default '0'

In this case strpos($sql, 'NOT NULL') is false, therefore the method would return null instead of 0.

[ausi]

In this case strpos($sql, 'NOT NULL') is false, therefore the method would return null instead of 0.

Which would be right, because null is the empty value for int(10) default '0'. Currently int(10) default '0' and int(10) NULL default '0' will produce different values even though they represent the exact same SQL column.

[leofeyer]

Then I guess the method should have been called getDefaultValueByFieldType(). The fact is that we are using the method to return 0 if the field definition is int(10) default '0'.

https://github.com/contao/core/blob/master/system/modules/core/classes/Versions.php#L291

[ausi]

But for getDefaultValueByFieldType() we should probably return the real default value, so that for int(10) default '123' the value 123 gets returned.

[leofeyer]

Maybe. Anyway, the purpose of the existing method is to convert an empty string into the suitable empty value depending on the field type. And we are expecting it to return 0 for numeric fields.

[ausi]

Then I don’t see the problem with my suggested change. All default '0' fields in the core do also have NOT NULL, so nothing changes here. But IMO int(10) default '0' and int(10) NULL default '0' should both return the same value to be consistent.

[leofeyer]

True.

[leofeyer]

Changed in cce5e51.

[aschempp]

FYI I'm using the same in the Doctrine integration: aschempp/contao-core-bundle@e7fc254#diff-ecdfb51ac371b19aded651b8b47ff381R138

You might want to use stripos though?

[leofeyer]

Changed in cff4c1a. I also applied strtolower() to $type.