Remove hardcoded plugin IDs from migration process plugins

I've closed #220449: drupal.org, no confirmation page for new user as a duplicate of this.

The Issue Summary over there gives a really clear example of the problems this causes. Adding credit for the useful issue summary.

Closed #3231983: Allow custom migration id config for d6_field_file process plugin as a duplicate. Adding credit'

The proposed resolution from the duplicate was, 'Default the plugin to use the d6_file migration, but allow a config option in the migration yml file when employing the d6_field_file process plugin.'

Created a POC of a DIRTY workaround for those of us who need an immediate solution until this is cleanly fixed. Marked it as WIP and named the branch "DIRTY" as it should never be committed.

diff --git a/core/modules/migrate/src/MigrateLookup.php b/core/modules/migrate/src/MigrateLookup.php
index 53f4246c8ba6b717dc5f2841dfc2c0859c460b2d..2892b4887e79aeec04ffe32b009e346a426d5fe8 100644
--- a/core/modules/migrate/src/MigrateLookup.php
+++ b/core/modules/migrate/src/MigrateLookup.php
@@ -35,6 +35,14 @@ public function __construct(MigrationPluginManagerInterface $migration_plugin_ma
   public function lookup($migration_id, array $source_id_values) {
     $results = [];
     $migrations = $this->migrationPluginManager->createInstances($migration_id);
+    // TODO: HACKY WORKAROUND to check for upgrade_$migration_id
+    // @see https://www.drupal.org/project/drupal/issues/3091841
+    if(empty($migrations)){
+      // If no migration with this ID was found, look for the same migration ID
+      // with "upgrade_" prefix:
+      $migrations = $this->migrationPluginManager->createInstances('upgrade_' . $migration_id);
+    }
+
     if (!$migrations) {
       if (is_array($migration_id)) {
         if (count($migration_id) != 1) {

https://git.drupalcode.org/project/drupal/-/merge_requests/1384.diff

@quietone, please don't kill me, I know this is very dirty and we need a conceptual solution.

@Anybody, no worries. It is not ideal to have such a hack but some people may need it and I would rather they be able to be productive than stuck.

This pulls out the migration_lookup from the BlockPluginId.php process plugin and places it in the migration yml where migrate-upgrade will find it.

The patch in #11 looks like a good start!

But we probably don't need to lookup against d6 and d7 migrations.

+++ b/core/modules/block/migrations/d6_block.yml
@@ -21,6 +21,24 @@ process:
+      migration:
+        - d6_custom_block
+        - d7_custom_block

d6_block should lookup against d6_custom_block and d7_block should lookup against d7_custom_block.

In a migrate meeting mikelutz was concerned about BC.

And yes, this will fail for anyone with a legacy generated yml.

This offers a way to be BC and fixes the point in #13.

Update list of process plugins.

Fix the coding standard error and add changes for FieldBundle.php

The remaining two are not as easy because the lookups are in for loops.

The remaining two are not as easy because the lookups are in for loops.

Do you have an idea of how you would approach it?

concerned about BC

Ah yes. Looks like you have addressed that nicely now!

Here's are my comments on the latest patch.

1. It doesn't look like this constant is used.

+++ b/core/modules/field/migrations/d7_field_instance.yml
@@ -9,6 +9,7 @@ source:
+    comment_node: comment_node_

2. What would you think about changing the code below such that we don't define _comment_type_source?

+++ b/core/modules/field/migrations/d7_field_instance.yml
@@ -25,6 +26,26 @@ process:
+  _comment_type_source:
+    -
+      plugin: explode
+      source: bundle
+      delimiter: comment_node_
+    -
+      plugin: extract
+      index: [1]
+      default: false
+  _comment_type:
+    -
+      plugin: skip_on_empty
+      method: process
+      source: '@_comment_type_source'
+    -
+      plugin: migration_lookup
+      migration:
+        - d7_comment_type
+      source:
+        - '@_comment_type_source'

It could be changed to this I think, which looks clearer to me.

  _comment_type:
    -
      plugin: explode
      source: bundle
      delimiter: comment_node_
    -
      plugin: extract
      index: [1]
      default: false
    -
      plugin: skip_on_empty
      method: process
    -
      plugin: migration_lookup
      migration:
        - d7_comment_type

Closed #3063273: BlockPluginId process plugin hard codes migrations breaking migrate_upgrade configuration as a duplicate, adding credit.

#3322542: Drupal 6 "d6_field_file" migration plugin static lookup, leads creating empty file fields was closed as duplicate. Take care if file fields not migrating in a Drupal 6 Migration, while files themselves are migrating!
Settings the priority to major here, as this issue affects many kinds of migrations using --configure-only, which is typical for larger migrations and searching the reason takes hours to days...
Just like it happened to us in that issue. once again.

Patch #17 applied successfully in drupal 10.1.x

Careful people, the dirty workaround from #8 doesn't work, as $this->migrationPluginManager->createInstances($migration_id) will NOT be empty, if an id wasn't found. Instead it will throw an \Drupal\Component\Plugin\Exception\PluginException.

Use attached patch for working hacky workaround.

UPDATE: Doesn't work!

Updating the issue summary to show what has been resolved already. Also I'll prepare a MR from the latest patch.

Sadly, I don't think I'm clever enough to fix the others :(

@quietone:

d6/FieldFile.php OK used in devValueProcessPipeline so will be in the yml

What does that mean? As of #28 it doesn't work. Drupal 6 file fields are not correctly migrated for us. It's hard-coded and with uprade_ it's broken!

See #3322542: Drupal 6 "d6_field_file" migration plugin static lookup, leads creating empty file fields

I updated the issue summary to be clear about the status.

I created a new branch, which is up-to-date with 10.1.x and applied patch #17 by @quietone, making it a bit easier for anyone to work on this issue! :)

This accepts the changes in #19. And adds potential fixed for BlockVisibility and FilterFormatPermissions. Not very pretty but I think it will work.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

It is more important that this is reviewed for the approach so setting back to NR.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Went to take a look at the MR but showing 1000+ changes which doesn't seem correct. Wonder if the MR could be fixed. Not sure what changes to look out.

Hiding patches as fix is now being worked on in MR 3198

Wasn't entirely too sure how to test.
Setup a fresh D10.1 site and D7 site
Installed the migrate modules

Did some edits to a User Login block on D7
But because of BlockVisibility.php in the IS
Changed the visibility and block title.
Ran the import and didn't receive any issues.
Block changes were imported correctly.

if there are additional changes please let me know and can test.

Rereading the IS I think the proposed resolution needs an update, which I have done.

There are no unanswered questions her, although I do not see a code review since #19, and more changes were added since then. Setting back to NR for review.

Did the testing in #45

I'm triaging RTBC issues. I read the IS and the comments, also noted that tests are not running regularly.

This is tagged for "Needs architectural review" but there is no such review, so setting to NR.

I also rebased the MR

Think a rebase went wrong as the MR is showing 1000+ file changes.

I have updated this to complete the support of previously generated d6_block and d7_block migrations.

Change record added.

And I did a more thorough self review. I found that this included changes for a different issue, so I removed those. I reworked logic in BlockVisibility and d6/FilterFormatPermission process plugins.

This still need subsystem maintainer review?

Appears feedback has addressed.

core/modules/block/src/Plugin/migrate/process/RolesLookup.php to add documentation I see there is example code

* Examples
*
* @code
* process:
* roles:
* plugin: roles_lookup
* migration: d7_user_role
* @endcode

I had an issue where block configs for a ton of my blocks, including content blocks, were failing to come in. MR #3198 fixed that problem, all my blocks that are content blocks migrated properly 🎉 A few remaining issues, and I don't know if they're related to *this* issue! -- and I don't have a ton of info yet, I just tried the patch on Friday, I'll continue hammering away at the remaining missing blocks this week and share more if I have more to share --

Meanwhile, my remaining issues that may or may not be relevant to *this* issue:

• "Seven" blocks came in with "seven" in the block machine name, but "claro" in the actual theme setting in the block.
  • In other words, the block machine names are, for example seven_user_login (but the theme setting is "theme: claro", as it should be -- so the blocks are properly configured, it's just the wrong theme name in the machine name).
  • Definitely not the end of the world, but, sharing!
• Views blocks didn't come in.
• Menu Block module blocks didn't come in.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Thank YOU @catch! Great to see this fixed finally :)