Default values for text fields may cause SQL errors [#101946]

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.

Comment	File	Size	Author
#9	content_admin.inc_6.patch	705 bytes	yched
#4	text.module_5.patch	588 bytes	yched

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

KarenS CreditAttribution: KarenS commented 7 December 2006 at 15:15

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

Comment #2

Heine CreditAttribution: Heine commented 7 December 2006 at 15:45

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.

Comment #3

yched CreditAttribution: yched commented 7 December 2006 at 21:54

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.

Comment #4

yched CreditAttribution: yched commented 7 December 2006 at 22:22

File	Size
text.module_5.patch	588 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

Comment #5

yched CreditAttribution: yched commented 7 December 2006 at 22:23

Status:

Active

» Needs review

Comment #6

KarenS CreditAttribution: KarenS commented 8 December 2006 at 10:54

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??

Comment #7

KarenS CreditAttribution: KarenS commented 8 December 2006 at 10:57

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.

Comment #8

yched CreditAttribution: yched commented 8 December 2006 at 13:38

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.

Comment #9

yched CreditAttribution: yched commented 9 December 2006 at 22:12

File	Size
content_admin.inc_6.patch	705 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.