Note the problem described in http://drupal.org/node/99005. It looks like in some cases, adding a default value to a text/blob field could cause SQL errors. Looking at the code in content_admin.inc, I'm not sure we're handling this correctly. It looks like we're providing a '' default value if none is supplied even for text fields. This needs investigation, but I don't have time to look into it now, so I'm posting this as an issue for whoever has time to work on it.

CommentFileSizeAuthor
#9 content_admin.inc_6.patch705 bytesyched
#4 text.module_5.patch588 bytesyched

Comments

karens’s picture

karens commented

BTW, the relevant function is content_db_add_column() which is the code that creates the db field columns.

heine’s picture

heine commented

MySQL doesn't support DEFAULT values on TEXT or BLOB columns. When running in strict mode MySQL 5 will complain (loudly). We just removed default values from core.

yched’s picture

yched commented

As a matter of fact, I was not aware of the post Karen mentioned, but I saw the core patch a few days ago, and investigated text.module.

From my tests, MySQL 4.1 was in fact rejecting ALTER TABLE `foo` ADD `bar` LONGTEXT DEFAULT 'baz' ;, but
accepted with no errors ALTER TABLE `foo` ADD `bar` LONGTEXT DEFAULT '' ; and ALTER TABLE `foo` ADD `bar` LONGTEXT DEFAULT NULL ; (the latter if the column is not specified NOT NULL, of course...)
So I did not touch anything in text.module, and simply added a comment saying 'this should not work but looks okay for now' (I did not commit that yet...)

But if MySQL 5 rejects it, I guess we have to correct this.

yched’s picture

yched commented
StatusFileSize
new text.module_5.patch588 bytes

Attached patch should correct this - before committing, I'd like someone with a MySQL 5 install to confirm, though...

Patch is for 5 branch, but should apply easily on 4.7

yched’s picture

yched commented
Status: Active » Needs review
karens’s picture

karens commented

yched, I was also looking at the content_db_add_column() function. I think we need a fix there so that default values don't get attached to text fields. That would keep any field module from inadvertently trying to provide a default value.

Also, I assume that providing no default value is also the right behavior for postgres??

karens’s picture

karens commented

And if we alter that function we can remove the default completely rather than trying to do default NULL, just in case that is a problem in some dbs.

yched’s picture

yched commented

Maybe you're right.
After all, it's a 'db-type' specific issue, and it's probably up to content.module to handle the db specifics.

I still think we have to keep the 'default' param in the field definition, though.

yched’s picture

yched commented
StatusFileSize
new content_admin.inc_6.patch705 bytes

Attached patch does the check in the mysql part of content_db_add_column().
(from the online reference, pgsql seems OK with default values for any column type)

This cancels my previous patch for text.module, which is not needed - and not wanted, aamof.

karens’s picture

karens commented
Status: Needs review » Fixed

Looks good to me. Committed. Thanks yched.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)