refactor code to compute the diff on an issue generated by a given follow-up

Here's a first attempt at doing this. I'd like to get some feedback before I spend much more time on this, to make sure I'm headed in the right direction.

This patch basically does what dww suggested in the original issue, but it adds a bit more data into the array and also creates a new hook that other modules can implement so that they can add to the diff output. This was done with the possible comment_alter_taxonomy module (see #36 in http://drupal.org/node/187480) in mind but might also be used by OG_project.

The functions mentioned in the original issue have changed since ifac was added, but this patch creates the function project_issue_comment_diff() which is called by both _project_issue_comment_table() and project_mail_generate_followup_mail_body() to get the differences between two comments or a comment and original issue. The patch does not call project_issue_comment_diff() from project_issue_comment() to validate the comment and make sure that at least something has changed, because I'm not sure that we want to go through all the code in project_issue_comment() to build the entire history of the issue and then diff the most recent version of all metadata with that submitted via the comment form.

I haven't heavily tested this but based on my limited testing it seems to work well.

This patch fixes a few things:
1. Changes name of new function to project_issue_comment_changes()
2. Changes name of new hook created by above to hook_followup_metadata_changes()
3. Hook functions called via module_invoke_all () can't use parameters passed by reference, so I added a global variable that's created in project_issue_comment_changes() that implementations of hook_followup_metadata_changes() can use.

The structure of the array returned by project_issue_comment_changes() looks like this:

Array
(
    [diff_keys] => Array
        (
            [0] => cid
            [1] => comment
            [2] => timestamp
            [3] => taxonomy
            [4] => files
            [5] => taxonomy_vid_4
            [6] => taxonomy_vid_3
        )

    [label_array] => Array
        (
            [pid] => Project
            [rid] => Version
            [component] => Component
            [category] => Category
            [priority] => Priority
            [assigned] => Assigned to
            [name] => Reported by
            [updator] => Updated by
            [sid] => Status
            [taxonomy_vid_4] => AdminTags
            [taxonomy_vid_3] => UserTags
        )

    [old] => Array
        (
            [cid] => 1129
            [comment] => 
            [timestamp] => 1195247267
            [taxonomy] => a:3:{s:4:"tags";a:2:{i:4;s:18:"at1, at2, at3, at4";i:3;s:23:"ut7, ut2, ut4, ut5, ut6";}i:6;a:4:{i:23;s:2:"23";i:21;s:2:"21";i:25;s:2:"25";i:20;s:2:"20";}i:5;s:1:"3";}
            [files] => 
            [taxonomy_vid_4] => at1, at2, at3, at4
            [taxonomy_vid_3] => ut7, ut2, ut4, ut5, ut6
        )

    [new] => Array
        (
            [cid] => 1130
            [comment] => test comment
            [timestamp] => 1195251661
            [taxonomy] => a:3:{s:4:"tags";a:2:{i:4;s:21:"at1, at2, at3, at4444";i:3;s:3:"ut2";}i:6;a:4:{i:23;s:2:"23";i:21;s:2:"21";i:25;s:2:"25";i:20;s:2:"20";}i:5;s:1:"3";}
            [files] => 
            [taxonomy_vid_4] => at1, at2, at3, at4444
            [taxonomy_vid_3] => ut2
        )

)

[label_array] is initially populated with the returned array from _project_issue_comment_labels() or project_mail_summary_fields(). I'm not sure how we'd populate the name fields in your suggested array structure. I agree that [diff_keys] isn't strictly necessary, but having that makes it much easier to see what fields have changed--otherwise, this would require stepping through [old] or [new].

BTW, with an addition to comment_alter_taxonomy and a quick patch to project_issue that stores taxonomy metadata in {project_issue_comments}, I've got the tagging stuff all working on my test site (see screenshot attached).

Given my reasoning behind structuring the changes array as I have, I'll set this back to cnr. If you can suggest an elegant way to get the field names in place while still using your array structure then I'll reconsider :)

I tested the patch. It applies cleanly (though with -p1 from within the module directory).
Follow up comments work as before.

I don't know how to set up a mail server on my own computer so that I can test the mail notification. Is that difficult to do (on a linux box)?

Is $project_issue_comment_project_issue_comment_changes_array a typo?

  // 1.  Use the global variable $project_issue_comment_project_issue_comment_changes_array and write
  //     any changes back to that variable.

Another typo just below in the same block, point 4. : changeserences

changeserences actually appears 3 times in the patch.
???

@beginner: Thanks for taking a look at this patch. I'll have to check into the potential typos you brought up. As for setting up a linux box to send emails, I use a linux box for development (Ubuntu 7.04) and I think it just came configured to send mail. If I did have to set it up for mail, that would have been over a year ago and I don't remember what I had to do to get it set up though.

I'll try to get an updated patch posted sometime today.

Maybe the mail notification does need a bit more testing.

I am running this patch on semi-production site that I just set up.
On a follow up, I received a mail with the following setting changes (while only 1 setting was actually changed):

-Projet:       
+Projet:       
-Version:      <rien>
+Version:      <rien>
-Composant:    
+Composant:    
-Catégorie:   <rien>
+Catégorie:   <rien>
-Priorité:    <rien>
+Priorité:    <rien>
-Affecté à:  
+Affecté à:  
 Signalé par: augustin
 Mis à jour par:augustin
-Etat:         actif
+Etat:         résolu

It's a shame because this issue is holding up that one: http://drupal.org/node/187480

@beginner: Are you sure you have your test site set up correctly? The emails I've gotten on my test site are correct and do not indicate that every field has changed, as you suggest. Can you add a deubgging print_r statement in project_issue_comment_changes() to see if the changes function itself is malfunctioning? I will hopefully have some time to work on this this weekend.

Thanks again for testing this.

I did some more testing. Removing the patch fixes the problem.

The problem occurs only with the 1st comment of every issue. It's logical: the array of issue setting has not been initialized yet.
There is no problem with any subsequent comment.

@beginner: Thanks for pointing out the typos. Those all came from a search and replace where a bit too much got replaced.

As for mail notification, I can't replicate your problem with a clean drupal install and a clean project_issue install with my patch from #10. I tried creating a new issue, the first reply (comment) to an issue, and additional replies and at no point did I get the werid diff you got. My only guess is that it has something to do with the fact that in your case it doesn't look like you have assigned a project or author to the issue.

Just as an example, here's an example of a new original issue:

 Project:      le
 Version:      <none>
 Component:    Code
 Category:     bug reports
 Priority:     normal
 Assigned to:  
 Reported by:  a
 Updated by:   a
 Status:       active

original issue

and here's the first reply to the issue.

 Project:      le
 Version:      <none>
 Component:    Code
 Category:     bug reports
 Priority:     normal
 Assigned to:  
 Reported by:  a
 Updated by:   a
-Status:       active
+Status:       patch (code needs review)

first comment.  Only changed status from active to cnr.

I did notice that in the new diff function I was getting PHP errors for the first issue, and so I added a bit of additional checking that gets rid of the errors. Maybe that also fixes the problem you're reporting?

I am going to test the patch soon.

What PHP version do you use?

@beginner: 5.2.3 on one system and 5.2.1 on another.

That might be the difference. I have php 4.4.6.
I am not an expert in those things, so I don't know if, why and how it is the cause.

My install is fresh. Drupal updated to 5.5, project* installed from CVS (D5-head) barely a week ago.

I experience the same problem with the (new) patch, while without it, everything seems normal.

Notice that none of the values are set:

-Project:      
+Project:      
-Version:      <none>
+Version:      <none>
-Component:    
+Component:    
-Category:     <none>
+Category:     <none>
-Priority:     <none>
+Priority:     <none>
-Assigned to:  
+Assigned to:  
 Reported by:  augustin
 Updated by:   augustin
-Status:       <none>
+Status:       <none>

If in line 391 of mail.inc I print_r the array, I notice that $comment_change['new'] is such that for each key, the value is empty.

I just tried my patch using PHP 4.4.7 and got the same results I get with 5.2.:

 Project:      le
 Version:      <none>
 Component:    Code
-Category:     tasks
+Category:     bug reports
 Priority:     normal
 Assigned to:  
 Reported by:  a
 Updated by:   a
 Status:       active

first follow up with php 4
changed task to bug

I'm not sure what's going on here.

@beginner

Add dpm($comment_changes); to line 386 of the patched mail.inc. That'll now look like this:

  // Check if the latest entry is actually the initial issue.
  if (empty($history)) {
    $content = $entry->body;
  }
  else {
    $previous = end($history);
    $content = $entry->comment;
  }
  $comment_changes = project_issue_comment_changes($previous, $entry, $fields);
dpm($comment_changes);
  // Mail summary (status values).
  $summary = '';
  foreach ($comment_changes['label_array'] as $field => $text) {

Create a new issue, then create a followup to the issue. Paste the printout of the array here. This is what mine looks like, using PHP 5.2:

Array
(
    [changes_keys] => Array
        (
            [0] => vid
            [1] => type
            [2] => status
            [3] => created
            [4] => changed
            [5] => comment
            [6] => promote
            [7] => sticky
            [8] => revision_timestamp
            [9] => body
            [10] => teaser
            [11] => log
            [12] => picture
            [13] => data
            [14] => category
            [15] => file_path
            [16] => file_mime
            [17] => file_size
            [18] => original_issue_data
            [19] => last_comment_id
            [20] => db_lock
            [21] => comment_form_location
            [22] => last_comment_timestamp
            [23] => last_comment_name
            [24] => comment_count
            [25] => taxonomy
            [26] => files
            [27] => cid
            [28] => timestamp
        )

    [label_array] => Array
        (
            [pid] => Project
            [rid] => Version
            [component] => Component
            [category] => Category
            [priority] => Priority
            [assigned] => Assigned to
            [name] => Reported by
            [updator] => Updated by
            [sid] => Status
        )

    [old] => Array
        (
            [vid] => 55
            [type] => project_issue
            [status] => 1
            [created] => 1197225178
            [changed] => 1197225218
            [comment] => 2
            [promote] => 1
            [sticky] => 0
            [revision_timestamp] => 1197225218
            [body] => test
            [teaser] => test
            [log] => 
            [picture] => 
            [data] => a:0:{}
            [category] => bug
            [file_path] => 
            [file_mime] => 
            [file_size] => 0
            [original_issue_data] => O:8:"stdClass":8:{s:3:"pid";s:2:"41";s:3:"rid";i:0;s:9:"component";s:13:"Documentation";s:8:"category";s:3:"bug";s:8:"priority";s:1:"2";s:8:"assigned";i:0;s:3:"sid";s:1:"1";s:5:"title";s:8:"original";}
            [last_comment_id] => 1
            [db_lock] => 0
            [comment_form_location] => 
            [last_comment_timestamp] => 1197225218
            [last_comment_name] => 
            [comment_count] => 1
            [taxonomy] => Array
                (
                )

            [files] => Array
                (
                )

            [cid] => 
            [timestamp] => 
        )

    [new] => Array
        (
            [vid] => 
            [type] => 
            [status] => 
            [created] => 
            [changed] => 
            [comment] => this is the first comment
changed bug to task
            [promote] => 
            [sticky] => 
            [revision_timestamp] => 
            [body] => 
            [teaser] => 
            [log] => 
            [picture] => 
            [data] => 
            [category] => task
            [file_path] => 
            [file_mime] => 
            [file_size] => 
            [original_issue_data] => 
            [last_comment_id] => 
            [db_lock] => 
            [comment_form_location] => 
            [last_comment_timestamp] => 
            [last_comment_name] => 
            [comment_count] => 
            [taxonomy] => 
            [files] => 
            [cid] => 15
            [timestamp] => 1197225218
        )

)

Fatal error: Call to undefined function: dpm() in mail.inc on line 386

What is dmp()?

I am going to try with print_r();

I apologize for all the trouble I am causing. I wanted to help this patch move along, not hinder it. My problem is a fairly minor cosmetic issue that you cannot even reproduce. If Dereck wants to commit the patch, go ahead :)

Anyway, I am going to test some more...

@beginner: Sorry, dpm() is in devel.module. It's basically the same as print_r() put displays the information in a more friendly way.

No need to appologize for causing trouble--I believe what you're saying, I'm just not understanding why you're seeing different behavior than I do. I'm sure we'll get to the bottom of this soon :)

Thanks :)

As mentioned earlier, I am not set up locally to test mails. The bug I am reporting has been tested and reproduced many times on the remote server. I updated project and project_issue one hour ago to the latest in DRUPAL-5 branch. So, I am up to date both locally and remotelly and I still could reproduce the bug. Drupal core has been updated recently. Still, there is the remote possibility that there are differences between the two setups. I can easily test print_r() kind of stuff locally. If need be, I'll test it on the remote server, too.

This being said, here is what I get locally on a one hour old Drupal install:

When I create a new issue:

Array
(
    [changes_keys] => Array
        (
            [0] => nid
            [1] => vid
            [2] => type
            [3] => status
            [4] => created
            [5] => changed
            [6] => comment
            [7] => promote
            [8] => sticky
            [9] => revision_timestamp
            [10] => title
            [11] => body
            [12] => teaser
            [13] => log
            [14] => format
            [15] => uid
            [16] => name
            [17] => picture
            [18] => data
            [19] => pid
            [20] => category
            [21] => component
            [22] => priority
            [23] => rid
            [24] => assigned
            [25] => sid
            [26] => file_path
            [27] => file_mime
            [28] => file_size
            [29] => original_issue_data
            [30] => last_comment_id
            [31] => db_lock
            [32] => comment_form_location
            [33] => last_comment_timestamp
            [34] => last_comment_name
            [35] => comment_count
            [36] => taxonomy
            [37] => files
        )

    [label_array] => Array
        (
            [pid] => Project
            [rid] => Version
            [component] => Component
            [category] => Category
            [priority] => Priority
            [assigned] => Assigned to
            [name] => Reported by
            [updator] => Updated by
            [sid] => Status
        )

    [old] => Array
        (
            [nid] => 
            [vid] => 
            [type] => 
            [status] => 
            [created] => 
            [changed] => 
            [comment] => 
            [promote] => 
            [sticky] => 
            [revision_timestamp] => 
            [title] => 
            [body] => 
            [teaser] => 
            [log] => 
            [format] => 
            [uid] => 
            [name] => 
            [picture] => 
            [data] => 
            [pid] => 
            [category] => 
            [component] => 
            [priority] => 
            [rid] => 
            [assigned] => 
            [sid] => 
            [file_path] => 
            [file_mime] => 
            [file_size] => 
            [original_issue_data] => 
            [last_comment_id] => 
            [db_lock] => 
            [comment_form_location] => 
            [last_comment_timestamp] => 
            [last_comment_name] => 
            [comment_count] => 
            [taxonomy] => 
            [files] => 
        )

    [new] => Array
        (
            [nid] => 4
            [vid] => 4
            [type] => project_issue
            [status] => 1
            [created] => 1197257053
            [changed] => 1197257053
            [comment] => 2
            [promote] => 1
            [sticky] => 0
            [revision_timestamp] => 1197257053
            [title] => sachoeu
            [body] => aue
            [teaser] => aue
            [log] => 
            [format] => 1
            [uid] => 1
            [name] => augustin
            [picture] => 
            [data] => a:0:{}
            [pid] => 1
            [category] => task
            [component] => Documentation
            [priority] => 2
            [rid] => 0
            [assigned] => 0
            [sid] => 1
            [file_path] => 
            [file_mime] => 
            [file_size] => 0
            [original_issue_data] => O:8:"stdClass":8:{s:3:"pid";s:1:"1";s:3:"rid";i:0;s:9:"component";s:13:"Documentation";s:8:"category";s:4:"task";s:8:"priority";s:1:"2";s:8:"assigned";i:0;s:3:"sid";s:1:"1";s:5:"title";s:7:"sachoeu";}
            [last_comment_id] => 0
            [db_lock] => 0
            [comment_form_location] => 
            [last_comment_timestamp] => 1197257053
            [last_comment_name] => 
            [comment_count] => 0
            [taxonomy] => Array
                (
                )

            [files] => Array
                (
                )

        )

)

Here is the 4th comment follow up in an issue:

Array
(
    [changes_keys] => Array
        (
            [0] => cid
            [1] => comment
            [2] => title
            [3] => timestamp
            [4] => files
        )

    [label_array] => Array
        (
            [pid] => Project
            [rid] => Version
            [component] => Component
            [category] => Category
            [priority] => Priority
            [assigned] => Assigned to
            [name] => Reported by
            [updator] => Updated by
            [sid] => Status
        )

    [old] => Array
        (
            [cid] => 5
            [comment] => aoeu
            [title] => aoeu',.p
            [timestamp] => 1197255734
            [files] => 
        )

    [new] => Array
        (
            [cid] => 6
            [comment] => comment 4
            [title] => comment 4
            [timestamp] => 1197257008
            [files] => 
        )

)

Here is the 1st comment follow up in an issue:

Array
(
    [changes_keys] => Array
        (
            [0] => vid
            [1] => type
            [2] => status
            [3] => created
            [4] => changed
            [5] => comment
            [6] => promote
            [7] => sticky
            [8] => revision_timestamp
            [9] => body
            [10] => teaser
            [11] => log
            [12] => picture
            [13] => data
            [14] => file_path
            [15] => file_mime
            [16] => file_size
            [17] => original_issue_data
            [18] => last_comment_id
            [19] => db_lock
            [20] => comment_form_location
            [21] => last_comment_timestamp
            [22] => last_comment_name
            [23] => comment_count
            [24] => taxonomy
            [25] => files
            [26] => cid
            [27] => timestamp
        )

    [label_array] => Array
        (
            [pid] => Project
            [rid] => Version
            [component] => Component
            [category] => Category
            [priority] => Priority
            [assigned] => Assigned to
            [name] => Reported by
            [updator] => Updated by
            [sid] => Status
        )

    [old] => Array
        (
            [vid] => 4
            [type] => project_issue
            [status] => 1
            [created] => 1197257053
            [changed] => 1197257087
            [comment] => 2
            [promote] => 1
            [sticky] => 0
            [revision_timestamp] => 1197257087
            [body] => aue
            [teaser] => aue
            [log] => 
            [picture] => 
            [data] => a:0:{}
            [file_path] => 
            [file_mime] => 
            [file_size] => 0
            [original_issue_data] => O:8:"stdClass":8:{s:3:"pid";s:1:"1";s:3:"rid";i:0;s:9:"component";s:13:"Documentation";s:8:"category";s:4:"task";s:8:"priority";s:1:"2";s:8:"assigned";i:0;s:3:"sid";s:1:"1";s:5:"title";s:7:"sachoeu";}
            [last_comment_id] => 1
            [db_lock] => 0
            [comment_form_location] => 
            [last_comment_timestamp] => 1197257087
            [last_comment_name] => 
            [comment_count] => 1
            [taxonomy] => Array
                (
                )

            [files] => Array
                (
                )

            [cid] => 
            [timestamp] => 
        )

    [new] => Array
        (
            [vid] => 
            [type] => 
            [status] => 
            [created] => 
            [changed] => 
            [comment] => comment 1
            [promote] => 
            [sticky] => 
            [revision_timestamp] => 
            [body] => 
            [teaser] => 
            [log] => 
            [picture] => 
            [data] => 
            [file_path] => 
            [file_mime] => 
            [file_size] => 
            [original_issue_data] => 
            [last_comment_id] => 
            [db_lock] => 
            [comment_form_location] => 
            [last_comment_timestamp] => 
            [last_comment_name] => 
            [comment_count] => 
            [taxonomy] => 
            [files] => 
            [cid] => 7
            [timestamp] => 1197257087
        )

The title field seems to be missing from the array, even though i changed it. But it's missing from yours, too.
I don't see anything significantly different.
I am going to test on the remote server.

Ehhhh... ???
On my remote setup, project_mail_generate_followup_mail_body() doesn't even seem to be called. When posting a new issue, i get the print_r() output, but when posting a follow up (1st, 2nd...), nothing is printed at all... But I do receive the mails...

@beginner: try installing devel.module and use dpm(). Your print_r() might be printed but then you're redirected somewhere else and you never actually see it. With dpm() I don't think that will be a problem.

Things are weird.
To be thourough, I added the following, so that I am sure something is outputed:

  echo '<pre>';
  echo '<p>previous: ';
  print_r($previous);
  echo '<p>entry: ';
  print_r($entry);
  echo '<p>fields: ';
  print_r($fields);
  $comment_changes = project_issue_comment_changes($previous, $entry, $fields);
  echo '<p>comment changes: ';
  print_r($comment_changes);
  echo '</pre>';

Locally, something WAS outputed on a new issue, a 1st comment and a 2nd.

On the remote server, something was output on a new issue, on a 1st comment but NOT on a 2nd comment ???
I receive all the mails nonetheless and always it is the 1st comment that has a weird formatting.

I tried a second time to make sure.
I have the expected output when posting a new issue, a 1st comment.
But I have no output on screen when posting a 2nd or 3rd comment but I still receive the mails.

I can try with devel.module as you suggest...

@beginner: Can you try commenting out the global line in project_issue_comment_changes(). I wonder if somehow that's causing problems.

Ok. I'll try that next.

Meanwhile, here is the output with dpm():

1st comment:

Array
(
    [changes_keys] => Array
        (
            [0] => vid
            [1] => type
            [2] => status
            [3] => created
            [4] => changed
            [5] => comment
            [6] => promote
            [7] => sticky
            [8] => revision_timestamp
            [9] => body
            [10] => teaser
            [11] => log
            [12] => picture
            [13] => data
            [14] => priority
            [15] => file_path
            [16] => file_mime
            [17] => file_size
            [18] => original_issue_data
            [19] => last_comment_id
            [20] => db_lock
            [21] => comment_form_location
            [22] => 0
            [23] => last_comment_timestamp
            [24] => last_comment_name
            [25] => comment_count
            [26] => taxonomy
            [27] => files
            [28] => cid
            [29] => timestamp
        )

    [label_array] => Array
        (
            [pid] => Projet
            [rid] => Version
            [component] => Composant
            [category] => Catégorie
            [priority] => Priorité
            [assigned] => Affecté à
            [name] => Signalé par
            [updator] => Mis à jour par
            [sid] => Statut
        )

    [old] => Array
        (
            [vid] => 39
            [type] => project_issue
            [status] => 1
            [created] => 1197261297
            [changed] => 1197261368
            [comment] => 2
            [promote] => 0
            [sticky] => 0
            [revision_timestamp] => 1197261368
            [body] => test33333333
            [teaser] => test33333333
            [log] => 
            [picture] => 
            [data] => a:6:{s:7:"contact";i:0;s:14:"picture_delete";s:0:"";s:14:"picture_upload";s:0:"";s:5:"block";a:3:{s:5:"forum";a:2:{i:1;i:1;i:0;i:1;}s:7:"comment";a:1:{i:0;i:1;}s:4:"user";a:3:{i:3;i:1;i:0;i:1;i:2;i:1;}}s:9:"operation";s:9:"add_proxy";s:6:"to_uid";s:3:"125";}
            [priority] => 2
            [file_path] => 
            [file_mime] => 
            [file_size] => 0
            [original_issue_data] => O:8:"stdClass":8:{s:3:"pid";s:1:"1";s:3:"rid";i:0;s:9:"component";s:4:"Code";s:8:"category";s:3:"bug";s:8:"priority";s:1:"2";s:8:"assigned";i:0;s:3:"sid";s:1:"1";s:5:"title";s:12:"test........";}
            [last_comment_id] => 1
            [db_lock] => 0
            [comment_form_location] => 
            [0] => 
            [last_comment_timestamp] => 1197261368
            [last_comment_name] => 
            [comment_count] => 1
            [taxonomy] => Array
                (
                )

            [files] => Array
                (
                )

            [cid] => 
            [timestamp] => 
        )

    [new] => Array
        (
            [vid] => 
            [type] => 
            [status] => 
            [created] => 
            [changed] => 
            [comment] => 1st comment 
            [promote] => 
            [sticky] => 
            [revision_timestamp] => 
            [body] => 
            [teaser] => 
            [log] => 
            [picture] => 
            [data] => 
            [priority] => 1
            [file_path] => 
            [file_mime] => 
            [file_size] => 
            [original_issue_data] => 
            [last_comment_id] => 
            [db_lock] => 
            [comment_form_location] => 
            [0] => 
            [last_comment_timestamp] => 
            [last_comment_name] => 
            [comment_count] => 
            [taxonomy] => 
            [files] => 
            [cid] => 48
            [timestamp] => 1197261368
        )

)

2nd comment:

Array
(
    [changes_keys] => Array
        (
            [0] => cid
            [1] => comment
            [2] => category
            [3] => title
            [4] => timestamp
            [5] => files
        )

    [label_array] => Array
        (
            [pid] => Projet
            [rid] => Version
            [component] => Composant
            [category] => Catégorie
            [priority] => Priorité
            [assigned] => Affecté à
            [name] => Signalé par
            [updator] => Mis à jour par
            [sid] => Statut
        )

    [old] => Array
        (
            [cid] => 48
            [comment] => 1st comment 
            [category] => bug
            [title] => 1st comment
            [timestamp] => 1197261368
            [files] => 
        )

    [new] => Array
        (
            [cid] => 49
            [comment] => aoeu
            [category] => feature
            [title] => 2nd comment
            [timestamp] => 1197261404
            [files] => 
        )

)

Mails are always the same...

I seem to be getting more consistent results with dpm() (i.e. I have some output each time).

Trying with:

function project_issue_comment_changes($old_data, $new_data, $field_labels = array()) {
  //global $project_issue_comment_changes_array;

No significant change that I could see.

I doubt it's relevant but... my install is 1 week old. There is only few core modules, project* and locale.module. I have translated some terms (as you can see) using the project_issue setting pages. That's about it. I actually tried once with locale.module disabled, but i got the same... so I really don't see.

@dereck: what do you think? I may have hit some very corner case and as long as I am the only one experiencing this, it's difficult to debug. I could do better testing on my own without bothering anybody if I could have a local setup with mail. But ftp'ing the files after each minor debug change is time consuming...
I suggest that this minor issue doesn't hold up this patch. If others experience it later, then we'd have a better basis to figure out what's wrong.

I'm moving on to other things until dww has had a chance to comment.

Thank you aclight for bearing up with me and guiding me :)

@beginner: Have you tried this patch out on a clean copy of the Drupal.org testing installation profile? That's what I've been using to test this patch, so it might be nice if you could try that as well since then we'd at least be using the same code. I just wrote a handbook page yesterday about how to use this profile. You'll need access to a CVS client, however, as there is not yet a release for the profile.

Also, what OS is each of your systems using? That might be relevant, though I am not sure how.

As for your testing environments, it sounds like you could optimize your work flow a little better. I don't know what text editor you are using, but one of the features I always look for in a text editor is the ability to read and write remote files via FTP or SFTP. I just open the file on the server remotely on whatever computer I happen to be using at the time. Any changes to the file are automatically uploaded back to the server when I save the file. That makes debugging a lot easier.

As for testing mail, this again depends on what OS you're using. I have a new development machine running MacOS. It doesn't appear to be able (out of the box) to send project_issue emails to a remote address, but I have set my test account's address to be username@loaclhost. I can then use the "mail" program from within Terminal.app to read messages sent by project issue. There is probably an even better way to do this, but my background is more PC/Linux oriented so I'm still figuring out how to do things on the Mac.

I think it's a bit premature to call this an edge case. As far as I know, you and I are the only person to test this patch yet. So potentially 1/2 of the people testing the patch are getting strange results. That's not a good thing :)

BTW, if you use IRC, feel free to ping me on #drupal.

@beginner: I'm sure you'll be glad to know that I am now able to reproduce this problem. I'm really not sure WHY I can reproduce it now and not before, however.

I'm setting the status to cnw until I can take a look and figure out where the problem is coming from.

@beginner: Do you by any chance have the core Book module enabled on your test site(s) where you're getting metadata diff problems in the emails?

Ok, I think I've fixed this problem. I'm not 100% sure why this is happening, but from what I can tell there is a conflict with the book.module (that may be related to this issue: http://drupal.org/node/98278) such that when book_nodeapi() fires, a property called '0' is added to the $node object. To see this for yourself, do the following:
1. Download and install a clean site using the Drupal.org testing profile.
2. Login to the site.
3. Subscribe yourself to all project issues.
4. Open up the project_issue/mail.inc file, and on line 385 add a dpm($previous); statement, so that you get:

....
  // Check if the latest entry is actually the initial issue.
  if (empty($history)) {
    $content = $entry->body;
  }
  else {
    $previous = end($history);
    $content = $entry->comment;
  }
dpm($previous);
  // Mail summary (status values).
  $summary = '';
....

5. Click on a project issue that has zero comments, click add a comment, put something in the comment text box, and click submit.

When dpm() prints out the node, towards the bottom you'll see this:

    [last_comment_id] => 1
    [db_lock] => 0
    [comment_form_location] => 
    [0] =>             <--- This is the problem.
    [last_comment_timestamp] => 1197323123
    [last_comment_name] => 

If you disable book.module, this won't happen.

So, now that I sort of figured why this was happening, fixing it is pretty easy. I just added the strict = TRUE parameter to in_array() in project_mail_generate_followup_mail_body():

    if (isset($previous) && !empty($comment_changes['changes_keys']) && in_array($field, $comment_changes['changes_keys'], TRUE) && $field != 'updator' && $field != 'name') {

This seems to fix the problem for me.

@beginner: Can you please test this updated patch and see if it also fixes the problem for you?

Ok! That's great :)
I'll test the new patch later today.
For now I can confirm that I had book.module enabled on the remote site.

You can indeed see the [0] => bit in my output above.

I tried patch #13 with the book.module disabled the bug didn't appear.

I tried the patch #33 with the book.module enabled and everything works fine.

The patch itself looks good. Typos have been fixed.

-> RTBC

What follows is blah blah... :)

@aclight
good job. It's good you were not as ready to give up as I was... :)
To answer your questions:
I am using Mandriva 2006, though I am looking forward to figuring out what prevents me from upgrading to Debian Etch. Until I do, I am stuck with php 4, mysql 4.0, etc. :(

I use vim and lftp. I have seen somewhere a patch for lftp so that it has vim support integrated. Thus I could do what you are saying (file editing by ftp). Maybe I'll try to patch and compile lftp myself once I've upgraded to Debian.
But it can only help that much: I am paranoid as far as ftp password is concerned. The very first web site (all html) that I have ever uploaded on a remote server - 7~8 years ago - was hacked only hours later because a guy sniffed my ftp login. Now, not having ssh, sftp nor anything secure at my host, I change my ftp password after each ftp opperation. So, debugging on a remote server would remain a combersome process.

So, for this and other reasons I do need to setup a minimal web server for testing mails. I have already installed the postfix and postfix-mysql rpm's but I am still not sure how to configure them. I'll write to the dev list about this. No need to reply here.

dpm() is cool. I should use devel.module more. I'll check the profile you're suggesting, too. Thanks.

Let's get this patch in, and back to the other issue : http://drupal.org/node/187480

Blessings,
:)

Simple? Since when is project* simple? :)

Attached patch changes the changes array to look like this:

* array(
* 'component' => array(
* 'name' => t('Component'),
* 'change_type' => 'modify';
* 'old' => 'Code',
* 'new' => 'User interface',
* ),
* 'sid' => array(
* 'name' => t('Status'),
* 'change_type' => 'modify';
* 'old' => 8,
* 'new' => 13,
* ),
* )

as suggested in comment #4. I did add another property, 'change_type', because it makes the logic about whether there is a change simpler to code and I think more readable. Plus, in the future, there might be a place to display 'add', 'delete', and 'modify' differently.

I'm not sure this really simplifies the code that much, but I did shorten some of the variable names so that helps out some.

@dww: is this closer to what you're looking for?

A) ... If there's no change, there shouldn't be a nested array for that attribute at all, IMHO..... Why bother?

This is true for the tables displayed on the issues when viewed via the web, but not true in issue emails, since some fields, such as Assigned to, Status, Reported by, and Updated by need to be printed regardless of whether the value of that field has changed.

But, as you requested, I took out the 'change_type' element of each nested array in project_issue_comment_changes(). Instead of checking for the value of 'change_type' I'm now just testing to see if 'new' and 'old' are defined, and if not that means there isn't a change and therefore those fields aren't printed.

B.) Agreed. I changed the name of project_mail_summary() to project_issue_change_summary(), and moved the function from mail.inc to issue.inc.

C.) Good idea. I've changed the hook call to use what you suggested, and that works fine.

D.) This has already been handled in #200097: Remove duplicate code for issue attribute labels.

I tested this pretty thoroughly and everything seems to be working well.

For anyone who wants to test this, I recommend using the Drupal.org testing installation profile at http://drupal.org/project/drupalorg_testing (CVS only).

For testing the new hook, I've also attached a simple .module and .info file that you can download and enable. The module is called "Fake testing module" and is in the Project group. All the module does is implement the hook added in this patch. NOTE: The test module will spit out the same thing every time. So, in other words, for each comment, you'll see a Vocabulary 10:, Vocabulary 11:, and Vocabulary 12: field, even if you don't have such vocabularies on your site or if they have nothing to do with project issues.

This is the same patch as #43, but I realized that since project_issue_comment_changes() isn't using a global variable anymore, the name of the variable doesn't have to be so unique since there are no namespace issues anymore. So, I shortened the name of the array to just $changes. Other than that search and replace, there are no changes in this patch.

i'd like dww to weigh in on my thoughts below before we go any further:

1. project_issue isn't even implementing the hook
2. i really don't think there's enough context here. ie, if i'm operating on a node, i expect the node object. likewise, if i'm performing an operation related to a comment, i would expect the comment object. with this implementation, the result of the hook can only be a blind diffing of the data. it makes sense that some modules will want to know what's going on w/ the comment before they do anything

Good point about adding the project_issue node into the call to project_issue_comment_changes(). Since the project issue node is already loaded in both places where picc() is called, I figured it made sense to just pass in the entire $node object, instead of passing in just $node->nid. I can change that to just $node->nid if you think that's better.

As for the comment id, that will always be available in $old_data and $new_data if we're looking at a comment. In the case where we're doing the diff between the original issue and the first comment. So, $old_data['cid'] would not exist as an element of the $old_data array, but $new_data['cid']=XXX would exist. At the point where picc() is called, the comment object is not necessarily loaded yet.

I talked with hunmonk in IRC about whether or not project_issue should implement this new hook itself. Because the metadata fields will be printed out on the table or email in the order they are in the $changes array, we really want project_issue to compute its changes first since we want those fields to be at the top of any display. Thus, I think it makes sense to NOT have project_issue implement this hook itself. This way we ensure that these fields are at the top of the array.

[Edit: There's also a new copy of fake_testing_module.module that implements the hook in the slightly new way. The fake_testing_module.info file in http://drupal.org/node/140473#comment-698876 can still be used.]

subscribing

code style looks good. the implementation is good.

however, i get some pretty funky output when i try to preview a comment that has metadata changes:

T:	T	» T
P:		» 
V:		» 
C:	C	» C
C:		» 
P:		» 
A:		» 
S:		»

couple of small things:

1. the phpdoc for project_issue_comment_changes() had some errors.
2. the metadata diffing wasn't being handled correctly for previews.

fixed both of those, and committed the attached patch to 5.x-2.x

code style looks good. gave it a pretty hard workover, seems to work for both web and mail output, with or without releases.

Agreed. Code looks good and behaves as expected.