Node.module does not fire event when a revision is deleted.

Better patch, gets rid of extraneous parentheses.

Some quick observations:

- move the node_load to just in front of the node_invoke_nodapi call, as it's unnecessary unless the if statement succeeds
- no need to comment that you are going to call node_invoke_nodeapi; just call it

The error about deleting the current revision seems odd. If I ask to delete the current revision, I should maybe be redirected to the node deleting page?

jvandyk, I'm not sure I understand your last comment about the delete part. It seemed intuitive to me how the patch worked, did you actually try it out yet? It worked as I expected it to work. I agree with your other comments though.

This is a lose end of the revisions patch that needs fixing. i am ok with the proposed patch. I think that Dries will object to
node_invoke_nodeapi($node, 'delete-revision');
and would maybe prefer
node_invoke_nodeapi($node, 'revision delete');

When I delete a revision, I get the following message:

"Deleted revision ()."

and in watchdog:

": deleted revision ()."

From a quick look at the code, it seems that the node is deleted before the messages are created.

Also, there's no 'delete-revision' in upload_nodeapi, so files associated with the deleted revision are never deleted.

Patch applies cleanly and works.

However, I might suggest that the watch dog message and drupal_set_message output the revision ID too... kind of confusing when it says "deleted revision test story" kind of confusing. Perhaps something like: "deleted test story revision 8" ... that would be a much more explantory message as to what exactly just happened, same for logging purposes.

Here's an updated patch. Ready to go!

Updated patch for killes comments, Dries hopefully this name works for you, works for us (forgot to change it in my last patch). We need this patch in so we can fix other bugs, thanks!!

does the hook need to pass along the id of the version that was deleted?

It needs to and does so by passing along the node object that got loaded with node_load($nid, $revision).

Haven't checked if it actually works, but should.

I have tested this patch and it does indeed work. Again, another patch holding up other patches!

Here is a patch that updates book and forum module. Tested.

Just to clarify, patch in comment 14 and the patch in comment 18 would *both* have to go in to fix the revision delete problem.

It looks like m3avrck's patch was added to the patch in http://drupal.org/node/35644 when it was committed:

http://cvs.drupal.org/viewcvs/drupal/drupal/modules/node.module?r1=1.551...

And, unfortunately, the patch is flawed. It allows you to delete the current revision if the URL is typed in directly.

That's correct but my patch for book and forum module need to be applied, too. And after that the change has to be documented.

I've patched HEAD (i.e. after m3avrck's patch was committed) to fix the problem I mentioned. node_revision_delete() will now check that the current revision isn't deleted, rather than checking that there's at least 2 revisions. I've also rolled in killes' patches for forum.module and book.module.

And rerolled from the root Drupal directory because I know it will make someone, somewhere, happy.

I did a fair amount of testing of the latest patch while working on http://drupal.org/node/31354. Everything seemed to work just fine.

I don't get how the problem mentioned in
http://drupal.org/node/31323#comment-56733

would be fixed by the last patch.

The latest patch checks that the revision being deleted isn't the current revision:

  ...
  $current_revision = db_result(db_query('SELECT vid FROM {node} WHERE nid = %d', $nid));
  // Don't delete the current revision
  if ($revision != $current_revision) {
  ...

Ah, ok, I had assumed a permission check would be missing. Good to go, I think.

What ever happened to the code in the patch I posted? http://drupal.org/files/issues/revision.patch ... that *was not* committed.

There is no: 'revision delete' in core, I think there might have been some confusion, I believe this patch needs to be updated.

Please read back through this issue, specifically at comment #20. m3avrck's patch was committed.

Ok, did not realize that part of my patch was already committed, that was very strange and confusing :-/

This patch is good to go then!

Wait, the patch in core has delete revision I thought it was to be: revision delete ? Which is it? Either way, the current patch won't work with core since it is backwards.

Fixed.

Looks good!

Dries committed http://drupal.org/node/31323#comment-56048, I will take Junyor's patch into http://drupal.org/node/30098