Project: Drupal Version: cvs Component: block.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: TDobes Status: patch It should be noted that the patch here contains a fix for a CRITICAL bug in block.module: At the moment, custom blocks cannot be deleted. A patch containing the fix alone (without other improvements) is located in another issue, which was marked as a duplicate [1] quite a while ago. Since we're now in a feature freeze, perhaps I should reopen the other issue so the deletion fix alone can be committed? [1] http://drupal.org/node/14625 TDobes Previous comments: ------------------------------------------------------------------------ December 9, 2004 - 15:07 : drumm Attachment: http://drupal.org/files/issues/block.module_2.diff (9.54 KB) * renamed 'boxes' to 'blocks_custom' * changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id * made associated changes in code to avoid 'box' and 'bid' * removed unique constriant on custom box titles (http://drupal.org/node/11724) * put the delete link for custom blocks back in, it was apparently lost in another update ------------------------------------------------------------------------ December 9, 2004 - 15:26 : jhriggs I think the only thing I would question is the renaming of bid. It seems inconsistent with the rest of drupal. We use nid, rid, uid, etc., etc. So, this should be a block ID, bid. ------------------------------------------------------------------------ December 9, 2004 - 16:05 : drumm Delta is the best name since this is what hook_block() calls it. Adding a bid (b as in block) to the blocks table is a good idea, but I wouldn't want to confuse things with a patch that is renaming bid in one context and adding bid in a related but different context. ------------------------------------------------------------------------ December 9, 2004 - 16:22 : Dries I prefer bid. I'd rather change delta to bid, instead of changing bid to delta. It is more consistent. Otherwise the patch looks good. ------------------------------------------------------------------------ December 9, 2004 - 16:43 : drumm Delta really isn't a good id. It isn't unique. The closest thing to a primary key is the blocks table is (module, delta). I think bid should be added as a real primary key on the blocks table. If there is a good word to use in place of delta that isn't bid, then that would be good to use. ------------------------------------------------------------------------ December 10, 2004 - 18:22 : drumm Updated patch which adds: * Added index to weight column in blocks table * Added primary key bid to blocks table * Replaced module/delta construct with bid where possible In additon to the previous: * renamed 'boxes' to 'blocks_custom' * changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id * made associated changes in code to avoid 'box' and 'bid' * removed unique constriant on custom box titles (http://drupal.org/node/11724) * put the delete link for custom blocks back in, it was apparently lost in another update ------------------------------------------------------------------------ December 10, 2004 - 18:23 : drumm Attachment: http://drupal.org/files/issues/block.module_3.diff (20.41 KB) And the patch too ------------------------------------------------------------------------ December 12, 2004 - 02:58 : Dries The upgrade path gave me problems. I got a fatal error: <?php $ret[] = update_sql('ALTER TABLE {blocks_custom} DROP INDEX title'); ?> The index did not exist on my setup. Weird. I got several hundred warnings about this line: <?php foreach ($blocks as $delta => $block) { ?> It's somewhat tricky, because you can't execute that update twice. My user and block tables are pretty unusable now. :) (Is the block->info field still being used? It looks quite redundant to me.) ------------------------------------------------------------------------ December 12, 2004 - 03:52 : Dries I ran this update on a copy of the drupal.org database and it seems to work. The reason my previous attempt failed, is because of the way error reporting was configured. My copy of the drupal.org database doesn't have an index on 'blocks_custom.title' either, but it did not result in a fatal error nor did it cause the upgrade script to halt. Warning are suppressed so they are simply not shown. In short, depending on how error reporting is configured, the upgrade path might or might not work. If it doesn't work, you're screwed because you won't be able to run the upgrade script again. At least, not without intimate knowledge of Drupal's internals. -- View: http://drupal.org/node/14158 Edit: http://drupal.org/project/comments/add/14158