Empty insert statements should fail gracefully

From the new DBTNG insert query docs:

In normal circumstances, if you do not specify a value for a given field and a default value is defined by the table's schema then the database will silently insert that default value for you. In some cases, however, you need to explicitly instruct the database to use a default value.

"This line instructs the the query to use the database-defined defaults for fields field1 and field2. Note that it is an error to specify the same field in both useDefaults() and fields() or values(), and an exception will be thrown."

As I understand, using db_insert('tablename') should be perfectly valid if there are defaults assigned to each field in the 'tablename' schema. That's not the case with the actions_aid table, but this probably shouldn't be applied globally as you're proposing. I'll let someone else chime in as well.

That's why I said "That's not the case with the actions_aid table". This should be fixed in those special cases where we should be using ->useDefaults, and not be throwing errors when it could be a potentially valid scenario. For example, we have a table

$schema['mytable'] = array(
  'field1' => array(
    'type' => 'int',
    'default' => '0',
  ),
  'field2' => array(
    'type' => 'varchar',
    'default' => '',
  ),
);

In this case, db_insert('mytable')->execute() should be valid and not throw an exception as you're proposing.

Using db_insert('test') should work just fine. There is an auto-increment field, and two fields with default vaules. I'd like input from db layer maintainers that we need a check for an auto-increment field default value before I continue this more.

After patch in #4, running Database Insert Defaults test: 18 passes 0 fail, 2 exceptions.
array_fill() [function.array-fill]: Number of elements must be positive Warning query.inc 66 InsertQuery_mysql->__toString()
implode() [function.implode]: Bad arguments. Warning query.inc 67 InsertQuery_mysql->__toString()

Looks like the DB layer should check if there are default fields specified. Attached patch checks count($this->defaultFields) in InsertQuery_driver::__toString(). With new patch, test passes without errors or exceptions.

Moving to the database component, as it relates to the API itself.

useDefaults() is the correct way to handle this sort of pseudo-sequence. It was added specifically because simpletest required that behavior. Check simpletest for the string "useDefaults" to see how it worked there.

Re #5, no, if you specify no fields at all then you get a syntax error currently, I think. :-) How we should behave in those cases that there are no fields defined (no-op, exception, etc.) is another matter. I'm a bit confused in this issue if we're discussing how to handle that case or how to avoid hitting that case in the first place. Both should be addressed.

I'll let Crell make this decision, but it looks to me that the current behavior is non-trivial to grok. Is there anything we can do to make this _easier_ to understand (in addition to fixing the problem)?

Well, I see two possible options:

db_insert('test')->execute() should:

1) Throw an exception. You're doing something dumb here that would create invalid code in the first place, so check for it, thrown an exception, and let the caller sort it out since he created the problem in the first place.

2) Do nothing and return NULL. An "empty" insert then becomes a NO-OP. On the one hand this is kinda what you'd expect: Insert nothing into {test}. It also means that looping for a multi-insert becomes safer, as the query object checks for empty arrays for you. Right now you must do:

if ($array_of_fields) {
  $insert = db_insert('test')->fields($field_names);
  foreach ($array_of_fields as $fields) {
    $insert->values($fields)
  }
  $insert->execute();
}

Making an InsertQuery with no fields into an internal NO-OP removes the outer if statement, since it happens inside the execute() method instead.

I can see valid arguments to both approaches. (#1 is "don't babysit broken code", #2 is "the code should do the hard work for you; that's why we write code.) Thoughts? I think I am somewhat leaning toward #2.

So after sleeping on it, I think there are no good cases where a merge query should be missing key fields, so that should throw an exception, but there is a valid use case for no-op insert statements, so those should be handled gracefully.

count($this->insertFields + $this->defaultFields)

I think in this case that is equivalent to count($this->insertFields) + count($this->defaultFields), but can we confirm that? I think I'd be more comfortable with the latter as it's a bit clearer what we're doing.

The code comment doesn't flow in English. Try this: "If no fields are specified, this method will do nothing and return NULL. That makes it safe to use in multi-insert loops."

Can we have a test case to confirm this behavior?

Better title.

Much better. Thanks!

Committed to CVS HEAD. Thanks!