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

Files: 
CommentFileSizeAuthor
#31 735900-block-delete_reroll-d6.patch3.18 KBFabianx
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es). View
#18 735900-block-delete-d6.patch3.26 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_3.patch. Unable to apply patch. See the log in the details link for more information. View
#16 735900-followup-d7.patch2.2 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_3.patch. Unable to apply patch. See the log in the details link for more information. View
#16 735900-block-delete-d6.patch3.26 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_2.patch. Unable to apply patch. See the log in the details link for more information. View
#15 735900-followup-1-d7.patch2.2 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-1-d7_0.patch. View
#14 735900-followup-1-d7.patch2.2 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-1-d7.patch. Unable to apply patch. See the log in the details link for more information. View
#12 735900-followup-d7.patch2.2 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_2.patch. Unable to apply patch. See the log in the details link for more information. View
#10 735900-followup-d7.patch1.64 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_1.patch. View
#8 735900-block-delete-d6.patch3.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_1.patch. Unable to apply patch. See the log in the details link for more information. View
#8 735900-followup-d7.patch1.64 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_0.patch. View
#7 735900-block-delete-d6.patch3.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_0.patch. Unable to apply patch. See the log in the details link for more information. View
#7 735900-followup-d7.patch1.67 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7.patch. View
#4 735900-block-delete-d7.patch3.23 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d7_0.patch. Unable to apply patch. See the log in the details link for more information. View
#3 735900-block-delete-d5.patch902 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d5.patch. Unable to apply patch. See the log in the details link for more information. View
#2 735900-block-delete-d6.patch966 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6.patch. Unable to apply patch. See the log in the details link for more information. View
#1 735900-block-delete-d7.patch2.45 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d7.patch. Unable to apply patch. See the log in the details link for more information. View
735636-cleanup-d7[2].patch847 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735636-cleanup-d7[2].patch. Unable to apply patch. See the log in the details link for more information. View

Comments

andypost’s picture

Issue tags: +needs backport to D6
FileSize
2.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d7.patch. Unable to apply patch. See the log in the details link for more information. View

Now patch with test, also this should be backported to d6

andypost’s picture

FileSize
966 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6.patch. Unable to apply patch. See the log in the details link for more information. View

Patch for D6

andypost’s picture

Issue tags: +Quick fix, +needs backport to D5
FileSize
902 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d5.patch. Unable to apply patch. See the log in the details link for more information. View
andypost’s picture

FileSize
3.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d7_0.patch. Unable to apply patch. See the log in the details link for more information. View

Added block_modules_uninstalled() to cleanup tables

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs review

For D6 there's no hook_modules_uninstalled() so only possible to fix custom blocks, patch from #2

andypost’s picture

FileSize
1.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7.patch. View
3.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_0.patch. Unable to apply patch. See the log in the details link for more information. View

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

andypost’s picture

FileSize
1.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_0.patch. View
3.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_1.patch. Unable to apply patch. See the log in the details link for more information. View

Queries was wrong now fixed

Status: Needs review » Needs work

The last submitted patch, 735900-followup-d7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_1.patch. View

Patch applies cleanly on local machine

Status: Needs review » Needs work

The last submitted patch, 735900-followup-d7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_2.patch. Unable to apply patch. See the log in the details link for more information. View

Same patch

andypost’s picture

Version: 6.x-dev » 7.x-dev

Maybe it's a version dependant

andypost’s picture

FileSize
2.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-1-d7.patch. Unable to apply patch. See the log in the details link for more information. View

Just to test queries in update hook

andypost’s picture

FileSize
2.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-1-d7_0.patch. View

So put this in install

andypost’s picture

Version: 7.x-dev » 6.x-dev
FileSize
3.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_2.patch. Unable to apply patch. See the log in the details link for more information. View
2.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-followup-d7_3.patch. Unable to apply patch. See the log in the details link for more information. View

back to 6 branch but now patches in order D7 them D6

Status: Needs review » Needs work

The last submitted patch, 735900-followup-d7.patch, failed testing.

andypost’s picture

Title: Deleting custom block does not clean block_role table » Deleting module's blocks when module is uninstalled
Status: Needs work » Needs review
FileSize
3.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 735900-block-delete-d6_3.patch. Unable to apply patch. See the log in the details link for more information. View

Changing title special for D6

not-anymore-used-9999’s picture

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.

andypost’s picture

Patch still valid for D6

carlos8f’s picture

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.

andypost’s picture

@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.

andypost’s picture

I think it's a bad idea to remove blocks in rehash because module could be just disabled not uninstalled

carlos8f’s picture

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.

carlos8f’s picture

Status: Needs review » Reviewed & tested by the community

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).

Gábor Hojtsy’s picture

Given that this patch drops stuff all around the blocks tables, it would be great to get some more testing.

andypost’s picture

@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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

@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.

andypost’s picture

Version: 6.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: -needs backport to D5 +Needs tests

@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

Fabianx’s picture

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).

Fabianx’s picture

Version: 8.x-dev » 6.x-dev
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es). View

Rerolleed patch against 6.x HEAD from #16 for review in #30.

andypost’s picture

guillermoig’s picture

#16: 735900-followup-d7.patch queued for re-testing.

guillermoig’s picture

#14: 735900-followup-1-d7.patch queued for re-testing.

guillermoig’s picture

#12: 735900-followup-d7.patch queued for re-testing.

guillermoig’s picture

#4: 735900-block-delete-d7.patch queued for re-testing.

guillermoig’s picture

#1: 735900-block-delete-d7.patch queued for re-testing.

guillermoig’s picture

735636-cleanup-d7[2].patch queued for re-testing.

Xano’s picture

Status: Reviewed & tested by the community » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.