Posted by bjaspan on November 7, 2007 at 1:53pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | bjaspan |
| Status: | closed (fixed) |
Issue Summary
Column blocks.pages has no default value and therefore must always be specified when inserting a new row. If it is not, MySQL uses an "implicit default value" but PostgreSQL properly fails the query.
The new _block_rehash, from http://drupal.org/node/80963, does not specify a value for pages so the query fails on that db.
Patch attached.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| block-rehash-pages.patch | 694 bytes | Ignored: Check issue status. | None | None |
Comments
#4
I deleted some previous comments of mine that turned out to be wrong.
The reason blocks.pages has no default value is that mysql does not allow default values on text columns. You can actually specify default '' when creating a column but if you then ask mysql what the column's default value is, it says it does not have one.
Confusion arises because of mysql's (IMHO stupid) "implicit default value" rules which will use '' as the default value for a text column if no other value is provided and the column is NOT NULL. This has allowed a lot of sloppy coding.
If we want to have a single database schema for mysql and pgsql and have schema.module able to verify that the database matches the schema, we cannot allow default values for text columns. Therefore, my patch to block.module is correct; a value for pages must be provided explicitly.
#5
Maybe add a short onliner above it like "// Empty value required by PostgreSQL" or something would be good, otherwise it looks puzzling.
#6
How would someone test this patch?
Enable a module that provides a block?
Edit the block admin page?
#7
You need to check whether pages is set and only if not then set default -- we want to let people to specify whatever in their hook_block.
#8
#9
#5 and #7: Fixed. New patch attached.
#6: To test this patch: Use pgsql. Display admin/build/blocks (or call _block_rehash()), removing any obsolete blocks from the db. Enable a new module that defines a block but does not specify a 'pages' value in the hook_block('list') output. Display admin/build/blocks (or call _block_rehash()) again. Without the patch, a query error results; with the patch, it does not. This could be done manually or via simpletest.
#10
I guess chx attached his patch while I was rewriting mine. They are functionally identical so it is fair to say we have reviewed each other's patches, so I'll agree with his RTBC. :-)
BTW, in this case I have to say it is mysql that sucks, not pgsql. mysql's "implicit default values" are a horrible idea that leads to sloppy coding and it is mysql, not pgsql, that cannot handle a default value on text columns.
#11
Thanks, committed.
#12
BTW same problem with the log field here: http://drupal.org/node/188462
#13
I need to document this to the Schema API handbook.
#14
In which case it is not fixed, and not critical anymore.
#15
Documented at http://drupal.org/node/190504.
#16
Automatically closed -- issue fixed for two weeks with no activity.