First of all, I'm marking this as a bug because I think it's a design oversight that needs to be corrected before 4.7 is release. I'm sure it'll get changed if people disagree.

I was looking information on the revision system when I came across a comment from someone confused by the changes. In there they asked for node_example.module to be updated to demonstrate revisions. Thinking that was a good idea I started on it but got bent out of shape when I realized that the only way to handle a revision being deleted is through hook_nodeapi().

I'm proposing adding a hook_delete_revision, the attached patch adds this and modifies book.module and forums.module to use it.

CommentFileSizeAuthor
#12 hook_delete_revision_50627.patch2.02 KBdrewish
#8 hook_delete_revision_0.patch2 KBdrewish
hook_delete_revision.patch2.79 KBdrewish

Comments

drewish’s picture

drewish commented

that patch no longer applies. i'll re-roll it though, i'd really like to get this in before the code freeze.

magico’s picture

magico commented
Status: Active » Needs review

This should have been marked as "needs review", or else no one will ever look at it!

drewish’s picture

drewish commented

whoops, yeah that would have been helpful. oh well, it totally missed the code freeze.

magico’s picture

magico commented
Status: Needs review » Needs work

It seems a simple patch to me. You could re-role it to HEAD and being important (as I think it is) it may get some attention from senior developers.

coreb’s picture

coreb commented
Version: x.y.z » 6.x-dev
Status: Needs work » Patch (to be ported)

Moving out of the "x.y.z" queue to a real queue.

coreb’s picture

coreb commented
Status: Patch (to be ported) » Needs work

I incorrectly used the new "patch (to be ported)" status.

pwolanin’s picture

pwolanin commented

This should certainly be implemented - re-roll for HEAD, and I'd give it a big +1

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new hook_delete_revision_0.patch2 KB

Here's a re-roll for HEAD, the book module now does everything through hook_nodeapi so I won't change it.

moshe weitzman’s picture

moshe weitzman commented

Is the intent here just to achieve better symmetry with the hook_insert/update/delete or does some module actually need this new hook as opposed to using nodeapi? if the latter, i suggest we add a hook when a need arises, and not earlier. not a big deal though.

drewish’s picture

drewish commented

moshe, it's both that and just making it obvious to developers how to support revisions. when i originally updated the example node module to support revisions it wasn't obvious to me why i needed to implement hook_nodeapi just for revisions. in my opinion a module should need to implement hook_nodeapi to do anything with its own nodes.

pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

patch applies with offset, seems to work and is a rational addition.

drewish’s picture

drewish commented
StatusFileSize
new hook_delete_revision_50627.patch2.02 KB

here's a re-roll that applies cleanly.

ChrisKennedy’s picture

ChrisKennedy commented

Still applies.

dries’s picture

dries commented
Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

I think we need to motivate this patch better. Why is this so important?

drewish’s picture

drewish commented

Dries, i don't know what more i can say that i haven't already said so i'll just summarize.

if you believe that modules should support revisions for nodes then it seems like you'd want to make that as easy as possible. it seems very counter intuitive that to correctly implement revision a module needs to implement hook_nodeapi. in my opinion a module should be able to do so by implementing hook_delete_revision rather than implementing hook_nodeapi to work with it's own nodes.

we've lived for two release without it so it's obviously not critical, but if we really care so much about having clean APIs, it would seem like we could clean up the warts in the ones we've got. if you don't think it's a worthwhile change feel free to mark it as won't fix.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

I'd like to see this go in, but it no longer applies.

preventingchaos’s picture

preventingchaos commented

This is essential to the consistency and maturity of the API. It will eliminate any confusion when creating node modules that deal with revisions, and increased consistency will give more credibility and an increased appearance of professionalism to Drupal as a whole. Developers will no longer have to use hook_nodeapi as the workaround for this bug, resulting in code that makes a little more sense, more respect for Drupal from prospective developers, and possibly more adopters and people joining the community.

casey’s picture

casey commented
Status: Needs work » Fixed

http://api.drupal.org/api/function/hook_node_revision_delete/7

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.