If you want the obsolete lines in block_roles to disappear after you deleted a role you need to resave the block.

However some modules depend on this table to do their access checks. For example homebox module does this by assuming if no roles are active then permission is granted. Something that is supported by core. However when deleting a role you ll look in the interface and see no roles selected. But for the module checking it, the lines attached to the deleted role will still be their. Resulting in a permission denied if the rid does not fit settings, which it wont because in fact it was deleted.
I think it is better to delete the roles in block_roles when a role is deleted. Like this you dont need to resave a block and other modules logic will still be valid. It keeps things clean.

Maybe allowing a hook here for other module to interact with wont be a bad idea either. In fact all modules that store rids should be given the chance to deleted their rid depending lines in the db. In the second patch there is an example where the block module could then implement the hook and delete its rid depending lines in block_roles.

Patch to delete the block_roles lines is attached.
Patch that invokes a hook attached.

CommentFileSizeAuthor
#16 delete-block-role.patch794 bytesdomidc
#15 hook-role-delete.patch1.29 KBdomidc
#14 hook-role-delete.patch1.29 KBdomidc
#12 delete-block-role-d6.patch794 bytesdomidc
#10 delete-block-role.patch808 bytesdomidc
#5 delete-block-roles-2.patch840 bytesdomidc
#4 delete-block-roles-1.patch840 bytesdomidc
#3 delete-blockroles.patch724 bytesdomidc
#3 hook-delete-roles.patch1.15 KBdomidc
hook-delete-roles-patch.txt1.15 KBdomidc
delete-blockroles-patch.txt724 bytesdomidc

Comments

domidc’s picture

domidc commented

Off topic: I just noticed patches here are tested automaticaly? Does this require the patch to be formatted in a certain way?

dpovshed’s picture

dpovshed commented

Hi domidc,

you're right about patch generation and format.

I feel this info is useful for you
http://drupal.org/patch/create

as well as here you can see auto test algorithm
http://drupal.org/node/332678

At a glance - the files you've submitted will not pass 'valid extension test', disregarding of contents. (.diff OR .patch are required)

Regards, Dennis

domidc’s picture

domidc commented
StatusFileSize
new hook-delete-roles.patch1.15 KB
new delete-blockroles.patch724 bytes

Trying to test the patches

domidc’s picture

domidc commented
StatusFileSize
new delete-block-roles-1.patch840 bytes

Again

domidc’s picture

domidc commented
StatusFileSize
new delete-block-roles-2.patch840 bytes

Trying once more

joachim’s picture

joachim commented

I'm not sure if D6 patches get tested, maybe only D7.

dpovshed’s picture

dpovshed commented
Status: Active » Needs review

Hi domidc!

I am setting status of this issue to 'needs review'

For more info - see - http://drupal.org/node/156119

This shall awake the testing bot.

However, according to my experience aand to this thread http://drupal.org/node/961172 , testing bot is in very bad mood these days :/

Status: Needs review » Needs work

The last submitted patch, delete-block-roles-2.patch, failed testing.

joachim’s picture

joachim commented

Maybe those Eclipse lines at the top have flummoxed the bot?

Also, small tweak needed:

+++ modules/user/user.admin.inc	7 Dec 2010 13:03:45 -0000
@@ -694,6 +694,8 @@
+    //Delete the roles from blocks_roles

Space needed after the //, and a final full stop too. Also, only one role is being deleted, so subject should be singular.

Powered by Dreditor.

domidc’s picture

domidc commented
StatusFileSize
new delete-block-role.patch808 bytes

Removed eclipse lines added the space but not sure what you mean with the full stop though?

joachim’s picture

joachim commented

A full stop at the end of the sentence.

domidc’s picture

domidc commented
StatusFileSize
new delete-block-role-d6.patch794 bytes

Yes of course.

domidc’s picture

domidc commented
Status: Needs work » Needs review
domidc’s picture

domidc commented
StatusFileSize
new hook-role-delete.patch1.29 KB

Here is the patch for the other possibility of solving this issue

domidc’s picture

domidc commented
StatusFileSize
new hook-role-delete.patch1.29 KB

Somehow previous patch got queued for testing.
Adding the other one to try and initiate testing for it.

domidc’s picture

domidc commented
StatusFileSize
new delete-block-role.patch794 bytes

Wrong patch

joachim’s picture

joachim commented

I don't think adding a hook_role_delete to D6 will fly... best stick to the other approach.

Status: Needs review » Needs work

The last submitted patch, delete-block-role.patch, failed testing.

domidc’s picture

domidc commented

It is still unclear to me wy the patch fails to be applied. Can someone explain?

@joachim can you explain why having a hook role_delete in D6 wont fly? Hook role delete is perhaps not a good idea. A thing like hook_roleapi with a delete operator is probably better. It think some modules would like to implement features when roles are created/changed/deleted? Now that is not really possible is it?

Status: Needs work » 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.