Term references should be incorporated into D6 node migration

+1. This will require changes to the d6_node builder and the d6_node source plugin, but those should be fairly straightforward.

Isn't it strange to move this logic into the node migration logic, since D6 sites can be set up without the Taxonomy module enabled?
In that case, the node migration would attempt to migrate D6 {term_node} data, which doesn't exist in the source.
Feels weird to me to have taxonomy-related migration logic in the node module.

Maybe I'm missing the point totally, but shouldn't the d6_term_node migration be reworked like this:
* Make sure to depend on node & taxonomy term migrations
* Source plugin gets the nid / tid combination from D6 {term_node}
* Process plugin uses "migration" plugins to process these source values
* Destination plugin doesn't save the node entity, but is just a custom destination plugin that adds the data to D8 {taxonomy_index} through the proper APIs
Or is that overly simplified? Haven't look into this very deeply, so I could be totally wrong :)

Would like to give this a shot if this can be clarified a bit more.

I just ran a D6-D8 migration and was wondering why my node's 'changed' (updated date) was set to the time of the migration, and not to the source field's value. Haven't done any detailed testing but it seems like the d6 term node migration actually sets the node's changed field when it runs and doesn't respect the actual source's 'changed' value which is set correctly during the d6_node__content_type migration. Need to investigate further and test but was searching and came across this issue so thought I'd mention it here in case it's relevant. Happy to create a new issue if necessary, and after being able to verify.

We discussed this in the weekly migrate meeting and think this can be resolved in a non-BC breaking manner.

Cross-posting. Fixing this would help with performance. Given a scenario where there are 10 vocabularies related to a node type, and there are 1000 nodes, we've set ourselves up for 1000 node saves for the initial node migration, plus an additional 10,000 node saves. A thousand for each vocabulary.

Needs tests, but here's a start. Marking NR to see what fails.

Next steps:
Add tests for new solution
Add deprecation notices to old
Remove old term node migration (but keep the source for BC)
Make sure there are BC tests for term node migrations

Hopefully fixes the test failures. With #2873395: [meta] Add @internal to core classes, methods, properties recently opened, I think we should mark D6NodeDeriver as @internal. If I don't hear any objections, I'm going to do that in my next patch.

I'm looking specifically at https://www.drupal.org/core/d8-bc-policy#classes to make my judgment call about marking @internal.

Let's hope that catching requirements will grab a bubbled up PNFE.

Updating credits to reflect sponsored work.

I don't know if it should be in a separate issue or not, but aren't there a number of migrations that are susceptible to this same issue?

e.g. the d6_upload migration also re-saves an existing d6_node causing the same performance problems and erroneously updating the timestamp.

The more of these things we run into, the more this will exacerbate the concern that svendecabooter highlighted above about including taxonomy (and file, etc.) migration data in the node migration, right?

#2934799: d6_term_node_revision migration ignores a lot of items that it shouldn't might give some ideas for test coverage here.

Rerolled the #25

 d8migrate.local git:(8.6.x) ✗ git apply -v 2597650-25.patch
Checking patch core/modules/node/src/Plugin/migrate/D6NodeDeriver.php...
Checking patch core/modules/node/src/Plugin/migrate/source/d6/Node.php...
Hunk #2 succeeded at 23 (offset 2 lines).
Hunk #3 succeeded at 182 (offset 2 lines).
Checking patch core/modules/node/src/Plugin/migrate/source/d6/NodeRevision.php...
Hunk #1 succeeded at 15 (offset 2 lines).
Applied patch core/modules/node/src/Plugin/migrate/D6NodeDeriver.php cleanly.
Applied patch core/modules/node/src/Plugin/migrate/source/d6/Node.php cleanly.
Applied patch core/modules/node/src/Plugin/migrate/source/d6/NodeRevision.php cleanly.</a>

Here's a re-roll and a small tweak to hopefully get fewer failures.

OK, now that it is green, it means we didn't break anything. So we need to add tests and figure out a way to deprecate the old migrations.

A few thoughts from working on this today. This is going to be tricky. Right now we don't break anything with this patch. But we don't fix anything yet either. To map the final field names in the d6 node deriver, we need the output of d6_vocabulary_field migration. But derivers run during plugin discovery. So we cannot derive the field names until later. But at that point, then we cannot inject the field names because derivers have already run. We could duplicate the logic that is being done inside of that migration. But that seems wrong.

  # This value is only used in the 'field_name' process pipeline below.
  raw_field_name:
    -
      plugin: migration_lookup
      migration: d6_taxonomy_vocabulary
      source: vid
    -
      plugin: skip_on_empty
      method: row
  field_name:
    # Prepend field_ to avoid conflicts with base fields, and make sure the
    # result is no longer than 32 characters.
    -
      plugin: concat
      source:
        - constants/field_prefix
        - '@raw_field_name'
    -
      plugin: substr
      length: 32
    -
      # This plugin checks if the vocabulary being migrated is the one used by
      # Forum. If so, we use the machine name that Forum expects. Otherwise, we
      # leave it unchanged.
      plugin: forum_vocabulary
      machine_name: taxonomy_forums

Perhaps better we encapsulate all the above into a new process plugin that can be called from both places. So we can call the same logic in both places using the same inputs. Which result in the same field name getting generated. And we can map things.

I just discovered migrate_drupal_migration_plugins_alter. And opened #3020329: [policy, no patch] declare the migration ymls in core subject to change per minor version.

The current plan here is to continue with the work and deprecate the term_node migrations. And in migrate_upgrade (contrib) filter them out. This let's us keep maximum BC because anyone using the core UI will still get the same result. But anyone using drush tooling can remove the extra step of term node migrations.

This needs a reroll to use the field discovery service, migrate_drupal.field_discovery.

This patch should fix the issue.

@shubham.prakash - Thank you for the reroll. It is adviced that once you reroll the patch you remove the "Needs reroll" tag.

The Migrate Drupal Module was approved for removal in #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3522602: [meta] Tasks to remove Migrate Drupal module and the removal work in #3522602: [meta] Tasks to remove Migrate Drupal module.

Migrate Drupal will not be moved to a contributed project. It will be removed from core after the Drupal 12.x branch is open.

Now that Migrate Drupal is deprecated this will not be fixed. Therefore, this task is closed as won't fix.

Thanks to all for working on this!