Simplify again the comment insert logic

I'll try to look at the patch this weekend.

I just glanced at the patch thus far. If two comments were to be created at nearly the same time, what happens? Just a few extra rows get written to the database but otherwise functionality is fine?

Any chance that the master/slave configuration used on d.o would cause problems with this method?

The patch in the original issue has the same problem that #314260: Strange behavior when comment_alter_taxonomy module is enabled after node is already created fixed. However, the attached patch fixes the problem by adding an additional check in comment_alter_taxonomy_project_issue_metadata(). Otherwise the patch is the same.

Patch in #3 doesn't work in the case where a node without any terms is created before the module is enabled and, after enabling the module, a comment is added that changes the terms. In this situation the addition of the term is not displayed in the comment the term was added.

Same patch but with a few modifications to the comment in _comment_alter_taxonomy_build_term_list().

Updated phpdoc for return value in _comment_alter_taxonomy_build_term_list().

-    $result = db_query(db_rewrite_sql('SELECT t.* FROM {comment_alter_taxonomy} cat INNER JOIN {term_data} t ON cat.tid = t.tid INNER JOIN {vocabulary} v ON t.vid = v.vid WHERE cat.nid = %d AND cat.cid = %d AND t.tid <> 0 ORDER BY v.weight, t.weight, t.name', 't', 'tid'), $nid, $cid);
+    $result = db_query(db_rewrite_sql('SELECT t.* FROM {comment_alter_taxonomy} cat INNER JOIN {term_data} t ON cat.tid = t.tid INNER JOIN {vocabulary} v ON t.vid = v.vid WHERE cat.nid = %d AND cat.cid = %d ORDER BY v.weight, t.weight, t.name', 't', 'tid'), $nid, $cid);
     while ($term = db_fetch_object($result)) {
       $term_list[$term->tid] = $term;
     }
+    if (empty($term_list)) {
+      // This cannot be empty because we save a dummy tid = 0 term even if the
+      // node was associated to no terms at the time the comment was saved.
+      // This can only mean that this module was not enabled at that time,
+      // so abort the diff.
+      return FALSE;
+    }
+    // Remove the dummy tid = 0 if set.
+    unset($term_list[0]);

In the case where the only record in {cat} for a comment is the row where tid=0, the db query above still returns nothing, because we're joining on {term_data}, which should never have a row with tid=0.

So, if a node has been created but has no comments, when the module is enabled and a new comment is created on the node, with terms added, the addition of terms on that node is not displayed in the diff table.

Ok, I think this finally works properly in all cases.

On the concurrency issue, I spoke with killes and he said that he didn't think this was particularly a big deal. The worst we're going to get is "Duplicate ID '34353' for query.."

But I wonder if we could remove the window for concurrency-related problems entirely (or at least make it MUCH smaller) by doing a subquery here?

INSERT INTO ... SELECT FROM ...

improved patch to simplify the "tid = 0" case.

This is now live on scratch and seems to work. There is one bug in the diff logic: if the module was enabled after the node was created, there is no entry in the cta table and the diff function returns FALSE. THis leads to the diff not being displayed for the first comment adding terms. it should return an empty array.