[drupal-devel] [bug] hook_delete_confirm -- custom controls for modules during delete cycle

[Bèr Kessels]

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

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.




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.