In the "making our database squeaky-clean" department:
The core schema has default values all over the place that make no sense. For example:
function node_schema() {
$schema['node'] = array(
'fields' => array(
'nid' => array('type' => 'serial', 'unsigned' => TRUE, 'not null' => TRUE),
'vid' => array('type' => 'int', 'unsigned' => TRUE, 'not null' => FALSE, 'default' => 0),
'type' => array('type' => 'varchar', 'length' => 32, 'not null' => TRUE, 'default' => ''),
node.type can never correctly be the empty string. node.vid can never correctly be 0. It is possible that node.vid needs to be 0 or some other temporary value while the node_revisions row is being created (since either node or node_revisions needs to come first, one column will always need a temp value), but that still doesn't mean that a "default value" of 0 is correct.
Having these incorrect default values works fine, of course, but it is semantically misleading. I'm pretty sure many of them are there because of MySQL's rather broken (IMHO) "implicit default values" behavior (http://drupal.org/node/159330) to make otherwise invaid queries work on pgsql.
Cleaning this up is straightforward. Remove the invalid default values, and any INSERT queries that rely on them will pop out during pgsql testing; they are then easy to fix. I suggest that these be considered bugs and fixed for D6, but if someone wants to tell me this is inappropriate during code freeze, I'm fine with pushing this to D7.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | schema-remove_incorrect_defaults-159346-6.patch | 62.27 KB | pillarsdotnet |
Comments
Comment #1
bjaspan commentedThis won't happen for D6.
Comment #5
Crell commentedNor in D7, apparently. Ah well.
Comment #6
pillarsdotnet commentedPatch
Comment #8
tr commentedThe patch shouldn't modify openid_update_6000(), and it should provide hook_update_N() for all the modules whose schema you've changed by removing the default.
Comment #9
pillarsdotnet commented(sigh)
Comment #10
pfrenssenMost of the changes to the defaults seem good, but I'm not sure about defaulting user.status to 1. I agree to remove system.update for D8, the chance that this is actually carried over from a D5 install and still has meaning is quite negligible, but then again maybe that's another issue.
I agree with #8 that openid_update_6000() should remain intact, but is it really necessary to write hook_updates for all tables to remove the defaults?
Comment #11
pillarsdotnet commented@pfrenssen
Unfortunately, yes. Which would make this huge patch many times larger. Which is why I pretty-much abandoned this issue as being more work than it's worth.
Comment #12
kattekrab commentedPerhaps time to move this one to D9 as well?
Comment #13
stewartsmith commentedAlso consider enabling strict_mode SQL_MODE in MySQL - this will help you catch a bunch of these things and likely improve portability to other RDBMSs that are a little more strict/correct than MySQL is.
Comment #14
Crell commentedstewartsmith: MySQL strict mode has been enabled since the early days of the Drupal 7 dev cycle, vis, about 5 years now.
Comment #23
quietone commentedOn the positive side there are only 19 install files that declare 'default' instead of 29, which is in the latest patch here.
I hope to remember to bring this issue up at the next Bug Smash initiative meeting.
Comment #24
quietone commentedThis came up in #bugsmash. larowlan thinks it is a task. I agree.
Comment #29
quietone commentedAnd now there are 13 files