Posted by andypost on March 8, 2010 at 11:52am
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | block.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | andypost |
| Status: | reviewed & tested by the community |
| Issue tags: | needs backport to D6, Quick fix |
Issue Summary
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
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| 735636-cleanup-d7[2].patch | 847 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 18,657 pass(es). | View details | Re-test |
Comments
#1
Now patch with test, also this should be backported to d6
#2
Patch for D6
#3
#4
Added block_modules_uninstalled() to cleanup tables
#5
Committed to CVS HEAD. Thanks.
#6
For D6 there's no hook_modules_uninstalled() so only possible to fix custom blocks, patch from #2
#7
As 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
#8
Queries was wrong now fixed
#9
The last submitted patch, 735900-followup-d7.patch, failed testing.
#10
Patch applies cleanly on local machine
#11
The last submitted patch, 735900-followup-d7.patch, failed testing.
#12
Same patch
#13
Maybe it's a version dependant
#14
Just to test queries in update hook
#15
So put this in install
#16
back to 6 branch but now patches in order D7 them D6
#17
The last submitted patch, 735900-followup-d7.patch, failed testing.
#18
Changing title special for D6
#19
Seems 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.
#20
Patch still valid for D6
#21
As 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.
#22
@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.
#23
I think it's a bad idea to remove blocks in rehash because module could be just disabled not uninstalled
#24
I 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.
#25
Tested 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).
#26
Given that this patch drops stuff all around the blocks tables, it would be great to get some more testing.
#27
@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
#28
@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.
#29
@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
#30
Back 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).
#31
Rerolleed patch against 6.x HEAD from #16 for review in #30.
#32
Related as cleanup #1382478: Cleanup {block_role} when deleting blocks