[drupal-devel] [bug] node_delete doesn't pass params to _delete
calls.
darrian
drupal-devel at drupal.org
Tue Jan 18 00:35:46 UTC 2005
Project: Drupal
Version: cvs
Component: node system
-Category: feature requests
+Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: darrian
Updated by: darrian
Status: patch
I switched this to bug report so maybe someone would look at it again.
Forgetting about the changes i've made to the event module, can
someone look at this patch to see if it's suitable to be put into
core?
I've looked at this (node.module, node_delete) code, and code that uses
this code and I cannot see anything wrong with my patch. It doesnt
affect the way anything works, it doesn't affect security, its very
small.
The only thing this patch does is increase the flexibilty of drupal and
gives more options to module developers.
Thanks,
Darrian
darrian
Previous comments:
------------------------------------------------------------------------
June 18, 2004 - 21:07 : darrian
Attachment: http://drupal.org/files/issues/node.module-cvs.patch (872 bytes)
When you delete a node, it asks for confirmation, after that step all
extra fields in $node get lost because $node is recreated with only 2
fields, nid and confirm. I created a small patch that adds extra
fields back into the $node variable so they get passed to subsequent
_delete calls.
------------------------------------------------------------------------
June 27, 2004 - 20:29 : Dries
AFAIK, deleting a node works fine. How can we reproduce your problem?
Not sure what you are trying to accomplish but this patch looks wrong.
------------------------------------------------------------------------
July 12, 2004 - 23:05 : darrian
Right, this patch is so the modification I made to event.module works.
I added functionality for repeating events to event.module, and in
order for the delete to work I had to make this small change outside of
event.module (believe me I really tried not to). What this patch does
is pass variables originally passed to the delete function to whats
called after the user clicks confirm.
In other words when a user clicks delete the first thing they get is a
prompt to confirm deletion, after the user clicks yes, the delete
function is called, but the only variables that are passed are nid and
confirmed, all other variables are lost, which is usually ok because no
other vars needed for deletion, however in my case I needed another
variable.
This patch has no effect on all current modules, the reason i made this
a bug report is because it seems to me like variables originally passed
to node_delete should still exist after the user clicks confirm, even
though current modules dont use other vars.
I made a reference to this bug from this bug:
http://drupal.org/node/view/4122
If the changes I made to event.module ever get put into the current cvs
version, then this patch also needs to get put in.
-Darrian
------------------------------------------------------------------------
July 13, 2004 - 07:28 : Dries
When an event (node) is deleted, the _delete hook is called and the
event (node) is passed as a parameter. That seems to work for all
other node modules/types so I don't understand why this shouldn't work
for the event module. What is so special about this particular field
or node type that this does not suffice?
------------------------------------------------------------------------
July 14, 2004 - 14:14 : darrian
$node is passed, but after the user clicks confirm, $node only has two
fields, nid and confirm. I need an extra field, which is whether I'm
supposed to delete only this event, or all events in the group. This
is for the reocurring events feature I added. My modification adds the
extra fields originally in $node back to $node after the user clicks
confirm.
-Darrian
------------------------------------------------------------------------
August 2, 2004 - 16:27 : darrian
In order for my repeating/reocurring event module patch to get submitted
to cvs, this very small patch needs to make it into core. It does not
effect the functionality of anything current.
Here's the details of what happens, >>> is what happens when my node
patch is there:
1. User is looking at the form to delete an existing event, there are a
bunch of fields like date/time title, etc. This holds true for any node,
but I am going to reference an event node.
2. User clicks delete to delete the node being looked at. (User checks
to delete all events in this group - from my event patch)
3. All the fields for that node are a form element at this point, some
hidden, like nid, and a variable I set telling the event_delete
function to delete all events in this group.
4. The code makes its way to the node_delete($edit) function. At this
point all the form elements from above are members of the $edit
variable, including my variable mentioned above.
5. The code checks to see if the user already clicked confirm, at this
point they haven't.
6. Now we're in the else branch that creates only two form elements,
the nid, and confirm. This has been ok in the past, because the only
thing that you needed to delete a node was its nid. But in my case I
want to delete multiple nodes, so this doesnt work.
>>> My node patch creates hidden form elements of all the previous
variables, not just nid and confirm.
7. The code makes it way back to node_delete, this time however there
are only two members of $edit, nid and confirm.
8. The code checks to see if the user has clicked confirm, and they
have.
9. Now we're in the true branch which deletes the core node components,
then calls the node specific delete function, in our case the
event_delete function, passing only the nid and confirm variables.
>>> My node patch makes it so all variables, including my varible to
check whether we're deleting all nodes or not, are passed back in.
10. Now we're in the event_delete function. The event delete function
checks for the variable to see if we're deleting all events in group,
it doesnt exist, so it acts as if the user didn't check to delete all
events, and deletes just this event.
>>> With my patch, all previous variables still exist, so event_delete
knows that it needs to delete multiple nodes.
Hopefully this patch gets into core before feature freeze for the sake
of the event module. If it doesnt, then my patch for
repeating/reocurring events also cannot make it into cvs.
Thanks, Darrian
------------------------------------------------------------------------
August 2, 2004 - 17:29 : moshe weitzman
the patch looks good to me. thanks for the clear description of the bug.
------------------------------------------------------------------------
August 13, 2004 - 08:03 : TDobes
+1, except the single comment prefixed with a # should be changed to //
------------------------------------------------------------------------
August 16, 2004 - 21:57 : darrian
Does the code freeze mean that this patch wont make it into 4.5?
-Darrian
------------------------------------------------------------------------
August 16, 2004 - 22:31 : Steven
This sounds like a bugfix or at least code fix, so it can still go in.
I'm wondering about your description though. It sounds that if I first
edit a couple of fields, then click delete, then my edited variables
are passed around rather than the original ones. Because I did not
click 'submit', this is not desirable.
It may be better to call node_load() before deleting the node, so the
correct, original data is passed to the nodeapi.
------------------------------------------------------------------------
August 17, 2004 - 01:09 : darrian
Remember, for node_delete, only nid and confirm are passed.
But I already do as you suggest, to be more specific:
node_load is called, those variables, along with extra fields that are
not loaded with node_load (such as my variable for deleting group
events), are passed to subsequent _delete calls.
Just to re-iterate, without this patch the ONLY variables that make it
to the final delete call are nid and confirm.
-Darrian
------------------------------------------------------------------------
August 17, 2004 - 01:55 : JonBob
The point is that (efficiency aside) if you have the node ID, you can
reconstruct the node with a simple node_load() call. If your delete
function is called with only $nid, you can use that to load the whole
node again if need be.
Now this may not be enough in your case, as you are presenting
additional interface to the user that adds fields to $edit that are not
part of the node, but all the parts *of the node* are certainly
available to you.
------------------------------------------------------------------------
September 5, 2004 - 17:47 : Steven
"The point is that (efficiency aside) if you have the node ID, you can
reconstruct the node with a simple node_load() call. If your delete
function is called with only $nid, you can use that to load the whole
node again if need be."
At the point where hook_delete gets called, the node itself has been
deleted already, so reconstruction is not an option. The point here
seems to be to pass variables from the editing form to the delete
operation. I'm not sure if this is a good idea.
If it's not about this, then maybe we should invoke deletes in the
opposite order: nodeapi, hook, node.module.
------------------------------------------------------------------------
September 9, 2004 - 23:05 : darrian
Right, the point is to get variables from the form passed to the delete
function, namely 1 variable, which tells me if I should delete 1 event,
or all grouped reoccuring events, that I need for the modification I
made to the event module.
The way I made the modification is very UN-obtrusive. No variables get
squashed, and everything current ignores the new variables.
Also, the variable that I want exists in the node_delete function the
first time around, its only after the user clicks confirm that all
variables except the nid are lost. So I could delete things with my
modified event module if i skip the confirm step, but I'm sure that
would bite someone down the road and I really dont want to do it that
way.
-Darrian
------------------------------------------------------------------------
October 4, 2004 - 21:06 : Steven
"Right, the point is to get variables from the form passed to the delete
function, namely 1 variable, which tells me if I should delete 1 event,
or all grouped reoccuring events, that I need for the modification I
made to the event module."
This sounds like really bad UI: the rest of the node form is about data
related to the node, which gets saved when you click "submit". Adding a
delete-related checkbox there sounds odd and confusing.
------------------------------------------------------------------------
October 8, 2004 - 21:58 : darrian
Attachment: http://drupal.org/files/issues/editevent.png (35.72 KB)
Here's the UI i'm referring to. This patch to node.module is extremely
small and unobtrusive... Why so much resistance to it?
------------------------------------------------------------------------
January 12, 2005 - 22:47 : darrian
Attachment: http://drupal.org/files/issues/node.module-4.5.1.patch (822 bytes)
Here's an updated patch for drupal 4.5.1
-Darrian
------------------------------------------------------------------------
January 13, 2005 - 00:28 : tangent
Is a recurring event a single node or a group of nodes? Without getting
into the appropriate implementation of that, your bug suggests that the
checkbox will allow multiple nodes to be deleted. This is why it has
been suggested that it is a confusing UI. It isn't necessarily clear
what the "group" of nodes is?
FWIW, if a recurring event is a single node (which is displayed in
multiple places on the calendar) then this would not be an issue.
------------------------------------------------------------------------
January 14, 2005 - 23:55 : darrian
A group is in fact multiple nodes. I did it this way for a few reasons,
one of them being other modules (such as remindme) work with it better
this way.
I am no longer working on development of this feature, so I am not
going to redesign the way its done. I only try and keep it updated so
other people that need this feature can use it, seeing as there is no
other alternative.
As for this particular patch to node.module I still think it is a bug
because variables available the first time any _delete function is
called dissappear after the confirm step. Those variables should be
available to module developers regardless of how they are used.
-Darrian
--
View: http://drupal.org/node/8611
Edit: http://drupal.org/project/comments/add/8611
More information about the drupal-devel
mailing list