PDO fails on db_add_field(): 'text'/'not null'

If I understand the issue correctly, it looks to me that the problem is that you aren't adding a default value. Once a table contains rows, you can't add a column that has a NOT NULL constraint unless you provide a default. This isn't a PDO issue (unless you count it as bad error reporting ;)


drupal=> CREATE TABLE foo ( bar text );
CREATE TABLE
drupal=> INSERT INTO foo VALUES ( 'test' );
INSERT 0 1
drupal=> ALTER TABLE foo ADD COLUMN blaz text NOT NULL;
ERROR: column "blaz" contains null values
drupal=> ALTER TABLE foo ADD COLUMN blaz text NOT NULL DEFAULT '';
ALTER TABLE
drupal=>

I'm almost certain this issue has come up in other threads. I'm pretty sure hswong has talked about it before. Please check the queue as I'm pretty sure there are existing threads that deal with this issue in some form or another.

@Pancho: This page in the Schema API docs seems to address this issue. Have you tried doing creating the field using the 'initial' parameter?

This sounds to me like it falls under "Don't babysit broken code" rather than an actual bug report.

Well we'd at least need to remove the broken code then, but I'm OK with a little better robustness in general. I agree it's not critical, though, so I'll meet you half way. :-)

@Crell: I'm not sure what you mean with "remove the broken code"?

- There is initial, and it's even documented in DatabaseSchema::addField():

  /*
   *   The specification may also contain the key 'initial', the newly
   *   created field will be set to the value of the key in all rows.
   *   This is most useful for creating NOT NULL columns with no default
   *   value in existing tables.
   */

- We also have some code to handle NOT NULL columns without a default value (I'm just assuming that works)

- The only thing I can see that we could improve is to throw an exception when we have the following three conditions: not null, no default and no initial value.

I mean if the issue here is that we're passing in schema definitions that are invalid, we should update those schema definitions even if we don't have runtime checks for valid schema definitions.

Should we be throwing an exception there, or just documenting it? I can go either way.

I don't find any invalid schema definitions in core?

$ grep -R db_add_field modules/ | grep -v default
modules/taxonomy/taxonomy.install:  db_add_field('taxonomy_vocabulary', 'machine_name', $field);
modules/taxonomy/taxonomy.install:  db_add_field('taxonomy_term_data', 'format', array(
modules/system/system.api.php:  db_add_field('mytable1', 'newcol', array('type' => 'int', 'not null' => TRUE, 'description' => 'My new integer column.'));
modules/poll/poll.install:  db_add_field('poll_vote', 'timestamp', $field);
modules/user/user.install:      db_add_field('users', 'picture_fid', $picture_field);
modules/user/user.install:    db_add_field('role_permission', 'module', $module_field);
modules/filter/filter.install:  db_add_field('filter_formats', 'weight', array(
modules/simpletest/tests/schema.test:      db_add_field($table_name, $column_name, $column_spec);
modules/comment/comment.install:  db_add_field('comment', 'created', array(
modules/comment/comment.install:  db_add_field('comment', 'language', array(
modules/comment/comment.install:  db_add_field('node_comment_statistics', 'cid', array(
modules/node/node.install:    db_add_field('node_revision', $column, array(
modules/aggregator/aggregator.install:  db_add_field('aggregator_feed', 'queued', array(

None of them is a text field and this can only happen when adding a field to a table with existing entries. It is perfectly fine to create a table with type text and NOT NULL.

So, imho the only thing we could do is to throw an exception as described in #8. It is already documented, the link is in comment #5.

Regarding throwing an exception, I'm not sure if it is necessary, since there are many other ways you can provide invalid schema data to db_add_field() and we don't check these either. The only difference here is that it is not so obvious what's wrong.

So to clarify then... The issue is "if you try to define an invalid schema directive, things break". I think that goes under "duh". :-)

So then the only question is whether we throw an exception or ignore it. I guess I have no strong preference.

Having just encountered this, I'd say throw an exception that better explains the problem.

I would also suggest changing this:

> $spec The field specification array, as taken from a schema definition. The specification may also contain the key 'initial'; the newly-created field will be set to the value of the key in all rows. This is most useful for creating NOT NULL columns with no default value in existing tables.

to say: 'this is *required*' rather than 'most useful'