When creating hook_diff() entries for various modules I ran into several issues:
1) There is no weight to order the diff entries
2) There is no way to alter a diff entry (such as a node's title)

Point 1) is not only for esthetics, but also for usability. You expect a diff to display in the same order as it appeared in the node's edit form.
Point 2) is important for instance on the audio module, where the title gets generated on insert. This becomes a problem if you wish to "preview changes." The solution? Have a hook_diff_alter() options.

Attached patch for the DRUPAL-5 branch adds #weight properties, normalizes the diff entries to be similar to drupal's form entries (or elements), and adds a hook_diff_alter() callback.

Example of using hook_diff_alter():

<?php

/**
 * Implementation of hook_diff_alter() for audio.module.
 */
function audio_diff_alter(&$old_node, &$new_node, &$diffs) {
  global $form_values;
  $op = isset($form_values['op']) ? $form_values['op'] : '';

  if ($op == t('Preview changes')) {
    $diffs['title']['#new'] = array(audio_build_title($new_node));
  }
}

?>
CommentFileSizeAuthor
#2 diff_hook_diff_alter.patch5.04 KBmoonray
diff_hook_diff_alter.patch5.43 KBmoonray

Comments

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

Conceptually, this looks good. A few code comments:
- remove the # before the property names in cck.inc and elsewhere. those are only needed infapi to distinguish from system properties. we don't do this in other api like structures.
- add a comment before hook_diff_alter with an example of how this can be used.
- is drupal_alter() in D5? if so, we should use that instead of your way of calling the hooks
- can you look into use theme(table) instead of own html? not required.

We have another huge patch in diff queue - http://drupal.org/node/125494. I'd really like to see that one go in before anything else. Help wanted.

moonray’s picture

moonray commented
StatusFileSize
new diff_hook_diff_alter.patch5.04 KB

Fixed up the patch.
- removed the # from before property names
- added comment to explain how hook_diff_alter() can be sued
- drupal_alter() is D6 only.
- I thought using theme(table) was part of the monolithic patch you just mentioned? If not, I think we can treat that as a separate issue, no?

I'll see what I can do to help with http://drupal.org/node/125494.

moonray’s picture

moonray commented
Status: Needs work » Needs review
dww’s picture

dww
we/he/they
commented
Version: 5.x-1.x-dev » 5.x-2.x-dev

Since this is a pretty big new feature, it should only go into 5.x-2.* (currently in HEAD), not 5.x-1.* (DRUPAL-5).

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

This looks quite promising. A few nits:

A) The patch has some funny characters: +Ê*

B) Shouldn't the API for hook_diff_alter() look like this:
$function($node_diffs, $old_node, $new_node);
?? Why should old_node or new_node be passed by reference? Seems like the convention with Drupal alter hooks is that the thing you alter by reference is the first arg, and the other args are just advisory. We don't want people actually altering the old or new node, right, just what diff thinks the changes are...

C) There's nothing in FAPI that's equivalent to what _diff_sort() is trying to accomplish? Seems like there must be a function in core already we can reuse for this. I could easily be wrong, but I wanted to ask.

Otherwise, this is great. Thanks!

moonray’s picture

moonray commented

A) I'll re-roll the patch soon, cleaning up those funky characters

B) In regards to the pass by reference, I was following conventions used in DIFF earlier. _diff_table_body(&$old_node, &$new_node) gets them passed by reference, so I figured I'd just pass it on the same way. Really, though, I think this has got to do with PHP4, since PHP5 passes objects by reference no matter what. We just want PHP4 to behave the same way as PHP5, so we pass by reference.
As for the order of the arguments, does it matter according to Drupal standards/convention? Either way, it's an easy fix.

C) There actually is a function like this in FAPI. Except that it uses '#weight', not 'weight' (which Moshe told me to remove). So... no there isn't one that I could find. :-)

I'll re-roll as soon as I get a response.

moshe weitzman’s picture

moshe weitzman commented

If we can reuse that fapi sort then I'll reverse my position and we'll stick with # character. Thanks for investigating.

moshe weitzman’s picture

moshe weitzman commented

B) I think Derek's convention makes sense. Lets break with the past and make this more sane.

moshe weitzman’s picture

moshe weitzman commented

now that the table themeing is in, it would be great to get a re-roll here with B changed as per dww suggestion. i can go either way on c)

moshe weitzman’s picture

moshe weitzman commented

now that the table themeing is in, it would be great to get a re-roll here with B changed as per dww suggestion. i can go either way on c)

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Fixed

committed and created new branch for D6, also committed to a new branch in D5.

the alter hook was deemed unnecessary.

committed by moonray and moshe at drupalcon boston 2008

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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