[drupal-devel] [feature] new nodeapi op 'delete pre' -- custom
controls for modules during delete cycle
shane
drupal-devel at drupal.org
Tue Aug 30 17:24:26 UTC 2005
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: feature requests
Priority: normal
Assigned to: thehunmonkgroup
Reported by: thehunmonkgroup
Updated by: shane
Status: patch (ready to be committed)
I'd like to see this go into 4.7 so that repeating events [1] can also
go live. I've been needing repeating events for over 3 years - and
have been manually adding repeating events every month.
HEARTY +1 for this.
[1] http://drupal.org/node/4122
shane
Previous comments:
------------------------------------------------------------------------
Wed, 13 Jul 2005 22:53:01 +0000 : 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 [2] 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.
[2] http://www.apartmentlines.com/hook_delete_confirm_screenshot.PNG
------------------------------------------------------------------------
Wed, 13 Jul 2005 23:00:56 +0000 : 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.
------------------------------------------------------------------------
Wed, 13 Jul 2005 23:28:28 +0000 : 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.
------------------------------------------------------------------------
Wed, 13 Jul 2005 23:30:08 +0000 : 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.
------------------------------------------------------------------------
Thu, 14 Jul 2005 01:03:55 +0000 : 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.
------------------------------------------------------------------------
Thu, 14 Jul 2005 10:08:40 +0000 : 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.
------------------------------------------------------------------------
Thu, 14 Jul 2005 15:46:07 +0000 : 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'.
------------------------------------------------------------------------
Thu, 14 Jul 2005 16:02:51 +0000 : Bèr Kessels
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)
------------------------------------------------------------------------
Thu, 14 Jul 2005 16:03:55 +0000 : killes at www.drop.org
I obviously support this patch. Aside from the demand for docs nothing
stops it from being submitted to core. Since the docs for hook calls
actually reside outside of core, even that is no reason to not commit
it right now.
------------------------------------------------------------------------
Thu, 14 Jul 2005 16:04:26 +0000 : Bèr Kessels
oh, btw, you have my +1. forgot to add that in the previous issue. ;)
------------------------------------------------------------------------
Thu, 14 Jul 2005 16:43:20 +0000 : thehunmonkgroup
>>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)
i definitely agree. it's usage confused me in the beginning and i
wrote it... ;)
if the patch gets committed, i'm happy to write supporting
documentation for hook_delete_confirm and post it whereever it needs to
go
------------------------------------------------------------------------
Thu, 18 Aug 2005 17:43:48 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/hook_delete_confirm_4.patch (2.19 KB)
ok, based on several discussions with core developers, i've expanded the
functionality so that mods can inject info into the admin mass delete
cycle as well. for single node deletions, the patch allows for
multiple modules to inject info into the confirmation screen. for
admin mass deletions the same capability exists, plus modules can also
inject data for multiple nodes. for the admin part of the hook, i also
cleaned up the code that prints the confirmation screen--removing some
unnecessary database queries and putting the list of nodes to be
deleted into a theme_item_list. because of the extra database queries i
removed, i think performance will be about the same even with the new
functionality. this patch introduces a very handy functionality, and
i've already found use for it in two of the mods i'm developing... :)
not only can it inject control structures to enable mods to handle
different types of multiple deletions (which i use in
eventrepeat.module), but it can also be used to inject custom module
messages into the delete cycle (ex., in signup.module, i use it to warn
users when they're about to delete a node that users are signed up for!)
for the module developer, this hook takes one argument, which is an
array of nids to be deleted, and returns an array, the elements of
which are also an array, with each key being the nid for which to
inject the data, and the corresponding value is the data to be
injected. (the array nesting was necessary because of the way
module_invoke_all returns it's results. this approach returns each
mod's custom injection data in it's own array--very flexible!)
you can see this hook in action here [3]--username 'test' password
'drupal'. please also note that i didn't spend much time on working
out the list theming, so we can clean that up later if it's not to
everyone's liking!
[3] http://drupal.org/www.outofprintdownloads.com
------------------------------------------------------------------------
Thu, 18 Aug 2005 17:48:24 +0000 : thehunmonkgroup
that last link to the test site was no good.
try here [4]--username 'test' password 'drupal'
also, i'll post some screenshots of the hook in action, and write up
some better doc in the next day or two...
[4] http://www.outofprintdownloads.com
------------------------------------------------------------------------
Sat, 20 Aug 2005 05:11:23 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/delete_confirm_screenshot.PNG (96.33 KB)
attached are some screenshots of what this hook can do.
after some more talks w/ other developers, it seems as though this
functionality may fit better in hook_nodeapi, as a 'delete confirm'
case. this seems to make sense b/c all modules have the option to
inject data into any of the nodes that are being deleted. i'm happy to
modify the patch in this case, but there is one catch that i wanted to
get some opinions on before i dive in...
in order for this hook to be supported in the admin batch delete, it's
necessary to pass an array of nids to the module hook, so that the
module can decide which nids to inject data into. the problem is,
hook_nodeapi doesn't currently have an available argument where that
can be passed in. what does everybody think about the idea of adding
an $extra argument? the hook would then look like:
hook_nodeapi(&$node, $op, $teaser = NULL, $page = NULL, $extra = NULL)
------------------------------------------------------------------------
Sat, 20 Aug 2005 06:03:26 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/delete_confirm_doc.txt (3.21 KB)
attached is module developer's documentation for this hook. If the code
gets put into nodeapi as i mentioned above, i can modify this
documentation accordingly.
------------------------------------------------------------------------
Sun, 21 Aug 2005 02:53:36 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/hook_delete_confirm_5.patch (2.54 KB)
after further irc discussions w/ chx, i've rewritten this hook to live
in nodeapi, and added a really cool extra feature... :)
now modules can actually prevent deletion of a node by explicitly
returning a value of FALSE. on a single node deletion, the user will
be bounced back to the node/edit screen, and in batch deletions, the
node will be removed from the batch delete queue prior to the
confirmation screen. i left it up to module developers to
drupal_set_message and tell their users what happened... :)
couple of things about this new approach:
1. it's less efficient than the old approach for batch deletions,
because now node_load and node_invoke_nodeapi get called for each node
in the batch, as opposed to the old way which just made one call for
all nodes. however...
2. it now lives in nodeapi, where i believe it belongs, and
3. the implementation for module developers is now way, way easier and
less complicated.
also, from an efficiency standpoint, i was able to completely remove
all database queries from the confirm screen code in admin batch delete
(the current approach queries the database once for each node in the
batch, which i thought was inefficient). so i don't think the overall
performance is going to be much different.
i'll post module developer code for this new version shortly. outside
of the delete prevention functionality, the patch does the same thing
as before--so the screenshots i posted above are still relevant.
------------------------------------------------------------------------
Sun, 21 Aug 2005 02:58:07 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/delete_confirm_doc_2.txt (3.61 KB)
new module developer documentation for the nodeapi version of this patch
attached.
------------------------------------------------------------------------
Sun, 21 Aug 2005 07:24:46 +0000 : chx
When a node calls node_view of other nodes, it's invaluable to be able
to stop the deletion of its 'siblings'. I was doing evil hackery
before: I str_replace'd in the theme to get rid of the delete button.
So I like this feature. However, I am not fully convinced that full
node object needs to be passed on during mass deletion. A simple
<?php
$node = new stdClass();
$node->nid = ...;
?>
should be enough IMO. But then again, node_delete issues a node_load.
For consistency reasons, maybe this should be changed. BTW. in HEAD,
node_load is simply node_load($nid).
So what others think: is node_load necessary before deletion?
What about renaming the op to 'delete pre'?
------------------------------------------------------------------------
Sun, 21 Aug 2005 07:26:47 +0000 : chx
Just a note, because I love giving credits: the implementation of how
deletions can be stopped is straight out from workflow.
------------------------------------------------------------------------
Sun, 21 Aug 2005 08:26:18 +0000 : Bèr Kessels
I beleive we need node_load. As Robin states: " i left it up to module
developers to drupal_set_message and tell their users what happened" So
it is a good thing to have teh complete node, including all the metadata
around it, available.
------------------------------------------------------------------------
Sun, 21 Aug 2005 14:16:28 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/hook_delete_confirm_6.patch (2.51 KB)
per chx's suggestions, changed node_load to node_load($nid), and renamed
the nodeapi $op to 'delete pre'. also double-checked the coding style.
unless anybody catches something else, i think this baby is ready to
go!
Ber: btw, the name is Chad... :)
------------------------------------------------------------------------
Sun, 21 Aug 2005 15:11:20 +0000 : Bèr Kessels
Sorry for the name, Chad. (what about two new profile fields: #drupal
irc nick and REal name?)
------------------------------------------------------------------------
Sun, 21 Aug 2005 15:46:16 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/hook_delete_pre_7.patch (2.59 KB)
minor cleanup. just adding a comment line to clear up potential
confusion--no functional changes
------------------------------------------------------------------------
Sun, 21 Aug 2005 16:08:17 +0000 : thehunmonkgroup
Attachment: http://drupal.org/files/issues/delete_confirm_doc_3.txt (3.6 KB)
...and...
updated module developer doc for the latest version of the patch
the updated patch can be seen in action here [5]
[5] http://www.outofprintdownloads.com
------------------------------------------------------------------------
Mon, 22 Aug 2005 23:05:23 +0000 : crunchywelch
+1 from me, a lot of thought and discussion has happened around it, and
I dont think there is another way to accomplish this without it. The
patch seems small enough and will allow much needed functionality to
progress. If only to quell the low roar of demand for repeating events,
this patch should be committed...
------------------------------------------------------------------------
Tue, 23 Aug 2005 00:35:11 +0000 : mathias
+1
The ability to cancel the deletion of a node comes in handy for
ecommerce when attempting to remove a still-valid recurring payment
product that still has subscribers.
------------------------------------------------------------------------
Wed, 24 Aug 2005 00:12:45 +0000 : humaneasy
+1 from me too. Nothing to add to the discussion besides thanks.
------------------------------------------------------------------------
Sat, 27 Aug 2005 04:55:20 +0000 : thehunmonkgroup
i've tested this patch extensively w/ no problems--works exactly as
advertised. code has undergone review and several revisions, and is
solid. developer feedback seems to indicate it's a desirable piece of
functionality. switching to commit status...
------------------------------------------------------------------------
Sun, 28 Aug 2005 06:56:58 +0000 : bomarmonk
Please get this in core for 4.7 (just adding my support, but I'm no
developer--- just anxious for this functionality).
More information about the drupal-devel
mailing list