Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.When custom block deleted http://api.drupal.org/api/function/block_custom_block_delete_submit/7
seems like someone forget about to clean block_roles table
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 735900-block-delete_reroll-d6.patch | 3.18 KB | Fabianx |
| #18 | 735900-block-delete-d6.patch | 3.26 KB | andypost |
| #16 | 735900-followup-d7.patch | 2.2 KB | andypost |
| #16 | 735900-block-delete-d6.patch | 3.26 KB | andypost |
| #15 | 735900-followup-1-d7.patch | 2.2 KB | andypost |
Comments
Comment #1
andypostNow patch with test, also this should be backported to d6
Comment #2
andypostPatch for D6
Comment #3
andypostComment #4
andypostAdded block_modules_uninstalled() to cleanup tables
Comment #5
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #6
andypostFor D6 there's no hook_modules_uninstalled() so only possible to fix custom blocks, patch from #2
Comment #7
andypostAs block.module required for D6 so cleanup added into install.inc
Also added hook_update_N() to cleanup orphan data.
D7 follow-up patch included
Comment #8
andypostQueries was wrong now fixed
Comment #10
andypostPatch applies cleanly on local machine
Comment #12
andypostSame patch
Comment #13
andypostMaybe it's a version dependant
Comment #14
andypostJust to test queries in update hook
Comment #15
andypostSo put this in install
Comment #16
andypostback to 6 branch but now patches in order D7 them D6
Comment #18
andypostChanging title special for D6
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedSeems to be what we had to implement for our uninstallation. Would be great if blocks from uninstalled modules would be removed automatically. Makes module development a bit easier and avoids useless data in the database.
Unfortunately, I can't test the patch at the moment, but as these are only few changes, there should not be any problems to expect.
Comment #20
andypostPatch still valid for D6
Comment #21
carlos8f CreditAttribution: carlos8f commentedAs far as I can tell, {blocks}/{block} doesn't need to be cleaned up manually, #235673: Changes to block caching mode not caught should clean that up on cache clear / update.php / cron etc. because it makes _block_rehash() part of hook_flush_caches(). I think the only issue then should be clearing {blocks_roles} on uninstall.
Comment #22
andypost@carlos8f - sounds good! But I think for consistency much better to remove all block-related data on module uninstall.
The right way is implement hook_modules_(installed, enabled and so) as it done in D7 because a lot of contrib lack this functionality.
But I scare that Gabor refuse this.
Comment #23
andypostI think it's a bad idea to remove blocks in rehash because module could be just disabled not uninstalled
Comment #24
carlos8f CreditAttribution: carlos8f commentedI don't think so, _block_rehash() needs to sync the {block} table with blocks currently implemented by _enabled_ modules (Drupal doesn't know or care what is implemented by disabled modules). If a block is no longer implemented by a module, it should disappear from {block} on the next rehash, likewise if a module changes a delta the table should reflect the change, not have the old delta hanging around. Also the block config page lists blocks returned by _block_rehash(), and we don't want to show blocks that are from disabled modules. I think _block_rehash() is working as intended, unless there is a crucial bug I'm missing.
Comment #25
carlos8f CreditAttribution: carlos8f commentedTested the patch, works pretty good. Was unsure about the subselect queries being kosher, but looks like some other parts of core use a similar approach.
re: #23, I guess it could be argued that you'd want to preserve your block settings if the providing module was disabled, then _block_rehash() could be changed to delete unimplemented blocks for enabled modules only, and leave the others intact. That might end up being marked 'by design', but if it seems like a legitimate bug, we could discuss it in a new issue (7.x then port to 6.x).
Comment #26
Gábor HojtsyGiven that this patch drops stuff all around the blocks tables, it would be great to get some more testing.
Comment #27
andypost@Gábor Hojtsy this queries are simple and straghtforward so I see no reason to test them. You could run this queries by-hand changing DELETE to SELECT
Comment #28
Gábor Hojtsy@andypost: so far, reviewers said the block removal happens on block rehash anyway, which is run when you disable modules, right? So why have an update function to remove stuff, that is not there anyway? I don't feel there is that big an agreement here, therefore I was asking for reviews.
Comment #29
andypost@Gábor Hojtsy I think we should back to 8.x bacause _block_rehash() deletes from {block} but does not touch {block_roles}
Also test should be updated (extended) to reflect on rehash
Comment #30
Fabianx CreditAttribution: Fabianx commentedBack to RTBC with #16 to 6.x as there is now a use case once #1173012: Blocks lose settings during update.php and cache clears has been applied.
Assuming #1173012: Blocks lose settings during update.php and cache clears is applied already:
Tested behavior (before this patch):
* Enable Module with block in code (no section assigned)
* Add block to section
* Disable Module
* Uninstall Module
=> Block is still set to section, and data is in DB
* Enable Module
=> Blocks shows up in section
FAIL
Tested behavior (with this patch):
* Enable Module with block in code (no section assigned)
* Add block to section
* Disable Module
* Uninstall Module
=> Block is removed from DB
* Enable Module
=> Blocks shows up with default configuration in no section
SUCCESS
=> Patch fullfills requirement of fixing the stated issue and removing tables, block settings, etc. from modules should be done in uninstall.
=> I say: Patch is ready to be applied.
Re-rolled patch is coming (against newest HEAD).
Comment #31
Fabianx CreditAttribution: Fabianx commentedRerolleed patch against 6.x HEAD from #16 for review in #30.
Comment #32
andypostRelated as cleanup #1382478: Cleanup {block_role} when deleting blocks
Comment #33
guillermoig CreditAttribution: guillermoig commented#16: 735900-followup-d7.patch queued for re-testing.
Comment #34
guillermoig CreditAttribution: guillermoig commented#14: 735900-followup-1-d7.patch queued for re-testing.
Comment #35
guillermoig CreditAttribution: guillermoig commented#12: 735900-followup-d7.patch queued for re-testing.
Comment #36
guillermoig CreditAttribution: guillermoig commented#4: 735900-block-delete-d7.patch queued for re-testing.
Comment #37
guillermoig CreditAttribution: guillermoig commented#1: 735900-block-delete-d7.patch queued for re-testing.
Comment #38
guillermoig CreditAttribution: guillermoig commented735636-cleanup-d7[2].patch queued for re-testing.
Comment #39
Xano#31: 735900-block-delete_reroll-d6.patch queued for re-testing.