NOT NULL fields need default value

But the schema API is not just target for MySQL, isn't? How about if other database need a default value for "NOT NULL" column?

IT IS a conflict if we don't have any default value for a NOT NULL column. What will be happened if we are calling INSERT/UPDATE from drupal_write_record(), but object with omit body and teaser? IT IS really happening: poll.module have a 'has_body' => FALSE attribute, so its object input for drupal_write_record() will ALWAYS without a VALID body and teaser.

This may cause some hidden bug, even it is now functioning for MySQL; provide a default value for NOT NULL is much logical ;-(

Patch tested for both MySQL and PgSQL with no error during install and modules installation. I grep all columns with "NOT NULL" where miss out default value, by using following script:

<?php

$array = array();

module_load_all_includes('install');
foreach (module_implements('schema') as $module) {
  $current = module_invoke($module, 'schema');
  foreach ($current as $table => $v1) {
    foreach ($v1['fields'] as $column => $v2) {
      if (isset ($v2['not null']) && $v2['not null'] && !isset($v2['default']) && $v2['type'] != 'serial') {
        $array[] = $table .".". $column .": ". $v2['type']; 
      }
    }
  }
}

print_r($array);

?>

I mark http://drupal.org/node/199161 as duplicated, as that is an real bug example of this issue. Here is its bug report in case of PostgreSQL:

Got the following page:
============
Page not found

* warning: pg_query() [function.pg-query]: Query failed: ERROR: null value in column "body" violates not-null constraint in /home/maximkr/drupal/includes/database.pgsql.inc on line 155.
* user warning: query: INSERT INTO node_revisions (nid, uid, title, teaser, log, timestamp, format) VALUES (14, 1, 'Do you like X', '* Yes * No ', '', 1197154493, 0) in /home/maximkr/drupal/includes/common.inc on line 3374.

Your Poll has been created.
The requested page could not be found.
=====

I mark this into my personal research project issue.

Please correct me if it is wrong:

1. The change should not break MySQL strict mode. The define of schema provide required information for developer (e.g. NOT NULL field should at least have default with xxx/yyy/zzz), but that's not means developer can/should simply omit its value when working with manual INSERT/UPDATE. This seems to be the duty of developer and application level design, but not schema.
2. No default value are added to serial fields, as script appended in #4. According to the logic of schema design, serial fields should NEVER handling manually: it should always maintained by DB internally.
3. NOT NULL field without DEFAULT value is always a conflict in schema design. We should just seems the default value of NOT NULL fields as a "final catch" before data are written into DB. The default value of "body" of a poll node need not to have any meaning (as we don't use it among application level); BTW, that is always meaningful for schema design: it is a NOT NULL field, so just give me something (but not simply ignore it).

On the other hand, if default value is documented within schema, helper API (e.g. drupal_write_record()) can simply fill in the omit information with valid value. It should simplify the difficulty of application design (as helper API really HELP the "catch and replace" handling).

IMHO, patch in #11 is all about application design fix. It is always truth and we should have this checking, too. BTW, adding default value for NOT NULL field is the duty of schema design; and this design shouldn't depend on ANY ONE of DB engine: if it is logically correct, apply it; BTW, if it is not support by XXX/YYY/ZZZ DB engine, omit and fix it within driver/schema implementation internally.

P.S. as I am now apply this "default value" logic to my personal research project, and it is now able to handle both MySQL/PostgreSQL/Oracle with same patched schema design. I am also working with IBM DB2 (good in progress), without retouch *any* schema. Hope this can be a reference about the correctness of patch in #4.

I guess the agree of point 1 is not something special: MySQL TEXT field ALWAYS need to OMIT default value, whatever we have/haven't schema API. This is the limitation for MySQL, but this is also a conflict for general and generic database schema design. This is always the developer's responsibility about INSERT/UPDATE correct information into DB, especially when we are handling NOT NULL fields.

Moreover, as now we having both schema API and drupal_write_record(), the correct and explicitly well defined default value for NOT NULL field can greatly increase the usability of these helper function. E.g. IF default value is ALREADY well defined for {node}, http://drupal.org/node/199161 won't appear. Again, this is not a new idea, let's review code from drupal_write_record():

<?php
    // For inserts, populate defaults from Schema if not already provided
    if (!isset($object->$field)  && !count($update) && isset($info['default'])) {
      $object->$field = $info['default'];
    }
?>

Recall the design of OSI 7-layers model: each layer should take their duty and responsibility correctly, in order to ensure the correctness of overall system. Patch in #11 take the responsibility in application level; on the other hand, patch in #4 take the responsibility of a correct database schema design and implementation. As Schema API is targeting for universal database abstraction, this is a CRITICAL logical bug if we don't have default value for NOT NULL fields correctly.

Patch in #4 is still valid for D6 CVS HEAD.

@bjaspan: That's why I don't hope to reply the question of #3 directly but waiting for the appear of http://drupal.org/node/199161: it will affect other database implementation, especially for Oracle and MSSQL.

In case of Oracle, it face a complicated difficulty when working with current schema implementation. This is because Oracle will handle empty string as NULL *incorrectly*. This will cause a critical error if we use = '' and != '' as inline query (in case of Oracle, we will need to use IS NULL or IS NOT NULL, which is totally incorrect). To help this issue, some special handling are needed to be taken within Oracle driver implementation INTERNALLY:

1. If fields come with empty string as default value, _db_create_field_sql() will catch and replace it as OCI8_EMPTY_STRING_PLACEHOLDER (in this case, chr(0) as dummy placeholder).
2. Don't use inline empty string SQL compare, but escape it as '%s'. db_escape_string() will catch it and replace as OCI8_EMPTY_STRING_PLACEHOLDER, so developer won't need to aware about this different between = '' and IS NULL.
3. Whenever fetching information by db_fetch_object(), db_fetch_array() and db_result(), replace OCI8_EMPTY_STRING_PLACEHOLDER as empty string. So Oracle driver will mask the special handling completely internal.

In case of MSSQL, according to the buggy .dll for connecting PHP with MSSQL, fetching an empty string from DB will ALWAYS result as a single space character: ' ', so the == '' check after fetching will always fail. Again, we can handle the mask-and-replace handling within MSSQL implementation, just as like as Oracle.

I guess we won't like to discuss this topic now, since both Oracle and MSSQL will not officially supported by D6. And that's why I start my personal research project, to test my idea on or before contribute back for D7, and so prove its correctness with solid production-level implementation.

Anyway, the general idea is: implement schema correctly without conflict nor database bias, and so each driver implementation can catch-and-replace as its suitable handling, internally.

@agentrickard: may I have some feedback for #15 and #16, before moving this issue to D7? Status critical, per #14.

I would like to give my final comment for the discussions of this issue in D6:

1. The case of PostgreSQL is similar as Oracle; according to my research, it is also MSSQL related. The auto-translation of NULL -> empty string (or better say, convert as a valid INSERT for TEXT) is a MySQL-specific issue. My patch in #4 shouldn't judge as a trade-off of the lowest common denominator; moreover, what we are now having IS a MySQL-bias handling, which conflict with the design of schema API.
2. drupal_write_record() will sometime seems buggy, when default value is not well defined, e.g. the case as http://drupal.org/node/199161. This helper function couldn't fully utilize its functionality, and so decrease its usability.
3. The complicated NOT NULL + w/o default value is too fancy, and require for a very clear mind in developing and application level debug. If the idea is simplify as "NOT NULL == with default value", the schema design will become more consistence and simple. As we can foresee about the widely use of drupal_write_record() among D6/D7, this database specific handling can be completely masked.
4. There is 36 lines of change in patch of #4, which also means that we have 36 fields need to be careful during INSERT/UPDATE EVEN we are now using drupal_write_record(), and need to preform AT LEAST 36 times of complicated application level debug handling. If schema is well defined and migrate the handling from manual INSERT/UPDATE to drupal_write_record(), case as http://drupal.org/node/199161 will not even happened.

I figure out this logical hidden bug with Oracle driver development, and try to report before it is become really harmful (Yes, it is even earlier than the announcement of http://drupal.org/node/199161. And finally, it is proved as harmful for PostgreSQL, too). According to complicated case as listed above, we can simply foreseeing that similar case will keep on reproduce among core and contribute modules within D6/D7, unless we accept for this "lowest common denominator" trade-off, whenever we are working with PostgreSQL or exploring for other database supporting possibility. This is not a threaten: it is a real case that really happening.

I have complete my duty and responsibility of reporting this bug, and propose a suitable handling which is proved as function among case of other database, too. I will come back for this in D7; BTW, I have no duty about similar issues which are causing by this miss-handling among D6.