Forum module can generate integrity constraint violations (duplicate primary key) when saving changes to existing non-default node revisions (can be triggered by d6_term_node_revision migrations)

Problem/Motivation

Repeatable issue I encountered trying to run a d6->d8 migration, although as far as I can tell the underlying problem is not specific to migrations. Interesting (but initially frustrating) issue to debug. The real problem is that Forum module does not always behave correctly when one attempts to save changes to an already-existing revision of a node (belonging to a forum) that is not the default revision.

I'm coming from a D6 site which has nodes that are part of a forum, and some of which are also tagged from the "Tags" taxonomy. I only get the database exception when I import the Forum taxonomy first, and then the Tags taxonomy; importing them in the opposite order does not generate the problem, for reasons I'll explain. (For the record, I'm testing these imports via drush, with the Migrate Upgrade contrib module, but I don't believe that's relevant in this case.)

The exception is getting thrown from "forum_node_update" (hook_node_update implementation) in forum.module. Relevant code from that function:

    // If this is not a new revision and does exist, update the forum record,
    // otherwise insert a new one.
    
    /** @var \Drupal\forum\ForumIndexStorageInterface $forum_index_storage */
    $forum_index_storage = \Drupal::service('forum.index_storage');
    if ($node->getRevisionId() == $node->original->getRevisionId() && $forum_index_storage->getOriginalTermId($node)) {
      if (!empty($node->forum_tid)) {
        $forum_index_storage->update($node);
      }
      else {
        $forum_index_storage->delete($node);
      }
    }
    else {
      if (!empty($node->forum_tid)) {
        $forum_index_storage->create($node);
      }
    }

The intention is, if we're saving a new revision of a node that belongs to a forum, we may need to insert a new record in the forum table; otherwise we may need to update (or possibly delete) an existing one. But all it does to check whether the node's revision ID matches the revision ID on $node->original - which is apparently always the "default" revision. Hence we'll also try to insert when we're saving an *old* revision (or more accurately, an existing revision that isn't the default).

Migrating term_node_revisions for the Forum vocabulary does not itself generate the problem (hence why I'm okay if I import it *after* the Tags vocabulary). Before migration, I don't have any entries in the forum table for the old (or rather non-default) node revisions, so the migration inserts them (the right action for the wrong reasons in this case), and all is well. The problem occurs if I then migrate term_node_revisions for the Tags vocabulary afterwards. Now there are already entries in the forum table for the non-default revisions, but when it goes to save those revisions again (even though it's not actually changing anything about the Forums, it's just trying to save the Tags references this time) forum_node_update will attempt to perform inserts again, because it's incorrectly assuming anything other than the default revision must be a new revision, and this time the database gets understandably upset with us for trying to insert a primary key it already has.

Proposed resolution

I'm not sure what the "best" solution would be here. A couple possibilities might include:

- Remove the comparison of $node->getRevisionId() and $node->original->getRevisionId() and instead look for a match of both nid and vid in the forum table to decide whether to update/delete or create

- Implement https://www.drupal.org/node/2509360, and pass this information to hook_node_update implementations so forum_node_update can react accordingly?

https://www.drupal.org/project/drupal/issues/2597650 would indirectly solve the problem for migrations specifically, but consensus does not seem to have been reached concerning the wisdom of that approach; furthermore, the bug in Forum module would remain, and could potentially be triggered by anything else that might have cause to perform a save for an existing node revision that isn't the default (Workbench in the contrib space comes to mind).