Highwatermark and file/node timestamp comparison are conflicting

[ctgraham]

Claude prompt:

I have a Drupal Migrate source plugin which is intended to examine Media of a certain media type ('findingaid'), and update the associated node with data extracted from the file which is referenced by the Media.

The migration uses the highwatermark property to detect changes in the source media files:

  high_water_property:
    name: file_changed
    alias: fc

I'm concerned that the source plugin is misconfigured, as an earlier design of the plugin included/excluded source records based on the comparison of the date when the file was changed vs. the date of when the node was changed. This seems to be evidenced by unexpected Total/Imported/Unprocessed counts for the migration.

Review the source plugin code to verify or refute this concern, and suggest updates as needed.

Claude response:

Your concern is well-founded — the two mechanisms are actively fighting each other, and that conflict is almost certainly the source of the odd Total/Imported/Unprocessed numbers.

Why this is a real problem

high_water_property: file_changed relies on Drupal core's built-in high-water logic in SourcePluginBase::next(). That logic works by keeping one global scalar in the key/value store — the maximum file_changed value seen across all rows in the last successful run — and skipping any row whose file_changed is at or below that mark.

Separately, shouldProcessMedia() does its own per-row filtering before the row ever reaches the iterator:

if ($file_changed > $node_changed) {
  return TRUE;
}
return FALSE;

This happens inside parseMediaXmlFiles(), so any row this method rejects never gets added to $this->parsedData — it doesn't exist as far as the source plugin is concerned.

That's the crux of the bug. SourcePluginBase::count() (used for "Total" in Migrate UI/Drush) counts whatever initializeIterator() produces. Since initializeIterator() calls parseMediaXmlFiles(), "Total" is not a stable count of source records — it's a count of records that currently happen to have file_changed > node.changed. That number necessarily shrinks after every successful run (because importing a node sets node.changed to "now," which is ≥ the file's timestamp), and can grow or shrink independently for reasons unrelated to the actual migration frontier (e.g., anything else that resaves the node). Meanwhile "Imported"/"Unprocessed" are derived from the map table and the high-water mark, which are tracking a different notion of "already handled." You end up with two independent, disagreeing definitions of "done," which is exactly the kind of drift that produces confusing counts.

There's a second, more subtle failure mode even setting aside the count confusion: because the high-water mark is a single global value, not per-item, if any one media item's file_changed is more recent than others, the stored mark advances to that value. On the next run, a different, unrelated media item that was genuinely updated — but whose file_changed is still below that global mark — will be silently skipped by core's high-water check, even though shouldProcessMedia() would have said "process it." High-water source plugins are only safe when the high-water field is effectively monotonic across the whole dataset (e.g., an autoincrement ID or an append-only timestamp), not when it's an independently-updatable "last changed" per unrelated item. Media file "changed" timestamps don't have that property.

What's actually redundant

You don't need the manual node-vs-file comparison at all:

• "Should this row be skipped because it's old?" — this is exactly what high_water_property is for (performance: skip re-parsing files whose file_changed is behind the frontier).
• "Should this row be re-imported because content changed since last time?" — this is exactly what Migrate's map-table row hashing already does. A row that's already in the map table and whose source data hash hasn't changed is skipped automatically; if the hash differs, it's treated as needing update — regardless of what the destination node's changed timestamp says.

The shouldProcessMedia() method is essentially a hand-rolled, incorrect reimplementation of both of these, using a comparison (file_changed vs. destination node.changed) that has no reliable relationship to either the high-water frontier or the row hash.

Suggested fix

Remove the destination-comparison filter entirely and let core handle it:

protected function parseMediaXmlFiles() {
  $media_storage = $this->entityTypeManager->getStorage('media');
  $query = $media_storage->getQuery()
    ->condition('bundle', 'findingaid')
    ->condition('status', 1)
    ->condition('field_media_file.target_id', NULL, 'IS NOT NULL')
    ->accessCheck(FALSE);
  $media_ids = $query->execute();

  if (empty($media_ids)) {
    \Drupal::logger('ead_migration')->notice('No media entities found with bundle "findingaid".');
    return;
  }

  foreach ($media_ids as $media_id) {
    $media = $media_storage->load($media_id);

    if ($media->hasField('field_media_file') && !$media->get('field_media_file')->isEmpty()) {
      $file = $media->get('field_media_file')->entity;

      if ($file) {
        $file_uri = $file->getFileUri();
        $file_changed = $file->getChangedTime();

        // No more shouldProcessMedia() gate here — every eligible media
        // entity becomes a candidate row. Core's high_water_property
        // and the migrate map/hash logic decide whether it's actually
        // (re)processed.
        $mime_type = $file->getMimeType();
        if (in_array($mime_type, ['application/xml', 'text/xml']) ||
            pathinfo($file_uri, PATHINFO_EXTENSION) === 'xml') {

          $xml_data = $this->parseXmlFile($file_uri, $media_id, $file_changed);
          if ($xml_data) {
            $this->parsedData = array_merge($this->parsedData, $xml_data);
          }
        }
      }
    }
  }
}

And delete shouldProcessMedia() (and the field_media_of node-lookup logic inside it) entirely — that check no longer serves a purpose once high_water_property and the map table are doing the job.

With this change:

• Total becomes the actual count of findingaid media with an attached file — stable and meaningful.
• Imported/Unprocessed come from core's map-table + high-water bookkeeping, which is what those columns are supposed to reflect.
• You still get the performance benefit you presumably wanted (skip re-parsing unchanged XML) via high_water_property, just implemented correctly.

One caveat worth testing for your data: since file_changed is per-item rather than a strictly append-only sequence, if you ever expect an older file to be legitimately re-touched after a newer file has already advanced the high-water mark, that older file's update could be missed. If that's a realistic scenario for you (rather than a theoretical one), it's worth dropping high_water_property altogether and just letting the row-hash mechanism in the map table do all the "did this change" detection — it's per-row and doesn't have the global-frontier problem, at the cost of the source plugin re-parsing every XML file on every run.