Issue status update for http://drupal.org/node/26649 Post a follow up: http://drupal.org/project/comments/add/26649 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: thehunmonkgroup Reported by: thehunmonkgroup Updated by: Bèr Kessels Status: patch 1: you are right about the batch deletion. In fact, that will even benefit from this, because it might be able to use this hook. 2: the fact that both Steven and I misread your patch shows that the use of this hook needs clarification. I am not sure how the hooks at drupaldocs are documented, but it really needs to show up there, with details about what to returns and what to pass along. Also some clarification about when this hook is called is important (for that caused the confusion at #1) Bèr Kessels Previous comments: ------------------------------------------------------------------------ July 13, 2005 - 23:53 : thehunmonkgroup Attachment: http://drupal.org/files/issues/node_module_hook_delete_confirm.patch (547 bytes) the proposed patch creates a hook_delete_confirm, which is called from node_delete during the confirmation cycle. this hook allows modules to inject custom control structures into the deletion process, as they don't currently have this ability. this is needed, for example, in the eventrepeat.module that i'm creating, which handles repeating/recurring events for event.module. without a solution such as this, there is no way for the module to put in options for different kinds of deletions--ex. 'this occurance only', 'this and all future occurances', 'all occurances'. click here [1] for a screenshot of what this hook looks like in action... i've tested this patch and it works great, even when multiple modules are calling hook_delete_confirm. i'd also be happy to write the documentation for hook_delete_confirm if this patch gets accepted. [1] http://www.apartmentlines.com/hook_delete_confirm_screenshot.PNG ------------------------------------------------------------------------ July 14, 2005 - 00:00 : thehunmonkgroup Attachment: http://drupal.org/files/issues/hook_delete_confirm_screenshot.PNG (92.79 KB) just a note that this patch will apply to both 4.6.x and HEAD. marking the version as 'cvs'. also, attached is the screenshot i mentioned above. ------------------------------------------------------------------------ July 14, 2005 - 00:28 : Steven a) New features don't get added to 4.6, this would only go into HEAD. b) You're using the historic argument order to implode(), and leaving out the 2nd argument, which is not allowed according to the PHP docs. It is best to be precise: http://php.net/implode. ------------------------------------------------------------------------ July 14, 2005 - 00:30 : Steven Also, you don't deal with mass-deletion. This needs to be added, otherwise things might break if your module depends on operations being performed on deletion. ------------------------------------------------------------------------ July 14, 2005 - 02:03 : thehunmonkgroup Attachment: http://drupal.org/files/issues/node_module_hook_delete_confirm_2.patch (551 bytes)
a) New features don't get added to 4.6, this would only go into HEAD.
no problem there. since many people are waiting for this feature, i'll include this patch (or a patched version of node.module) so that the functionality will be available for those that wish to begin using my module right away in 4.6.x
b) You're using the historic argument order to implode(), and leaving out the 2nd argument, which is not >>allowed according to the PHP docs.
attached patch corrects this.
Also, you don't deal with mass-deletion. This needs to be added, otherwise things might break if your >>module depends on operations being performed on deletion.
this shouldn't be necessary, as the modules have ample tools to deal w/ mass deletion themselves--as long as they can pass some kind of control stucture into the deletion cycle (so that the module knows what kind of delete operation it needs to perform). i've tested the multiple delete functionality extensively in eventrepeat.module, and it works flawlessly with this patch in place. my thinking in designing this patch was add the absolute minimum to core that allows the modules some control in the deletion process. i think this is all that they'll need as long as they're well written. ------------------------------------------------------------------------ July 14, 2005 - 11:08 : Bèr Kessels I do like the idea of this, since i ran into a similar issue for shazamgallery and for books. But, steven is right when he says you should deal with batch deletion. in node/admin you can delete a range of nodes at once. your patch will break that. And furthermore, I would like to see a good piece of documentation on how to use this. From what you have coded it seems the hook must return an array. Why not a straing? You concenate arrays with nothin ('') which IMHO is bad PHP. But I guess you have good reasons for returning a string. ------------------------------------------------------------------------ July 14, 2005 - 16:46 : thehunmonkgroup
But, steven is right when he says you should deal with batch deletion. in node/admin you can delete a >>range of nodes at once. your patch will break that.
not true. this patch isn't involved in the actual deletion of nodes, only with allowing modules to pass in a control structure so they have the ability to initiate different kinds of deletes--this doesn't interfere w/ the admin batch delete functionality at all. admin batch delete has it's own custom delete function, and this hook will not be called during it. here is a detailed use case for this patch, so that the flow can be seen more clearly: in eventrepeat.module, i have a need to be able to give the user a choice to delete entire repeat sequences, or just the current node. this needs to be decided at time of deletion, and i want to give them three choices: 'this occurance' 'this occurance and all future occurances' (which deletes from the date of the selected node forward) 'all occurances' (which deletes from today's date forward) prior to this patch, there was no way that my module could inject this kind of control structure into the deletion process. here's the hook i put into eventrepeat.module to take advantage of the new feature provided by this patch: function eventrepeat_delete_confirm($node) { if (variable_get('event_nodeapi_'. $node->type, 'never') != 'never' && variable_get('eventrepeat_nodeapi_'. $node->type, 0) == 1) { if ($node->eventrepeat_rid) { $form .= form_hidden('eventrepeat_rid', $node->eventrepeat_rid); $options = array( 'this' => t('This occurance only'), 'future' => t('This occurance and all future occurances'), 'all' => t('All occurances') ); $form .= form_radios('Repeat event--delete the following:', 'eventrepeat_delete_type', 'this', $options, $description = t('\'This occurance and all future occurances\' will delete repeat events from the date of the selected node forward, \'All occurances\' will delete repeat events from today\'s date forward.'), $required = FALSE, $attributes = NULL); } return $form; } } this injects the id of the repeat sequence and the control structure i detailed above into the delete confirmation screen (i do some checks at the top to make sure that this data is only injected for nodes that are in a repeat sequence). now, when the user clicks 'Delete', the info i injected is available as $_POST data. since the user clicked delete, drupal processes the deletion of the node, and eventually during that process eventrepeat_nodeapi is called, at which point the module grabs the info it needs from $_POST. if, for example, the user selected 'all occurances', the module then calls it's own custom delete function to handle that process. simply put, the guts of that process is grabbing all the nids that need to be deleted, and calling node_delete for each one (in this case i pass in the confimation setting as well to node_delete, to automate the process). node_delete takes care of all the deletion dirty work for each node. the only tricky part is making sure that my custom delete function isn't called from eventrepeat_nodeapi multiple times (b/c that's called multiple times in a multiple delete)--i take care of this by unsetting the injected $_POST data when my custom delete function is called. works beautifully... :)
And furthermore, I would like to see a good piece of documentation on how to use this.
hopefully the above example makes usage clear, but i'd be happy to write up more doc if needed.
From what you have coded it seems the hook must return an array. Why not a straing?
the hook returns an html string which is the form data that will be injected into the confirmation screen
You concenate arrays with nothin ('') which IMHO is bad PHP. But I guess you have good reasons for >>returning a string.
these steps are necessary, because: #1 module_invoke_all returns an array, and each element of the array is an html string that i described above. if multiple modules return form data to be injected into the confirmation screen, this array will have multiple elements #2 the $extra argument in theme confirm needs to be passed in as a string, so i implode the array. it's best to include the 'glue' argument in implode (according to PHP documentation), but i don't want a seperator--i just want to turn the array into a string. therefore i insert an empty string as the 'glue'.