Issue #3125763 by quietone, heddn, dww, benjifisher, Lal_, Neslee Can…

…il Pinto, alexpott, mikelutz: Hidden dependency on migrate_drupal from node module when only migrate.module is enabled

modules/migrate/tests/modules/migrate_no_migrate_drupal_test/migrate_no_migrate_drupal_test.info.yml

@@ -0,0 +1,8 @@
+name: 'Migrate No Migrate Drupal Test'
+type: module
+description: Provides fixture for testing without migrate_drupal.
+package: Testing
+version: VERSION
+dependencies:
+  - drupal:migrate
+  - drupal:node

modules/migrate/tests/modules/migrate_no_migrate_drupal_test/migrate_no_migrate_drupal_test.routing.yml

@@ -0,0 +1,7 @@
+migrate_no_migrate_drupal_test.execute:
+  path: '/migrate_no_migrate_drupal_test/execute'
+  defaults:
+    _controller: '\Drupal\migrate_no_migrate_drupal_test\Controller\ExecuteMigration::execute'
+    _title: 'Execute'
+  requirements:
+    _access: 'TRUE'

modules/migrate/tests/modules/migrate_no_migrate_drupal_test/migrations/node_migration_no_upgrade.yml

@@ -0,0 +1,22 @@
+id: node_migration_no_migrate_drupal
+label: Node Migration No Migrate Drupal
+source:
+  plugin: embedded_data
+  data_rows:
+    -
+      id: 1
+      nid: 1
+      title: Node 1
+    -
+      id: 2
+      nid: 2
+      title: Node 2
+  ids:
+    id:
+      type: integer
+process:
+  nid: nid
+  title: title
+destination:
+  default_bundle: no_migrate_drupal
+  plugin: entity:node

modules/migrate/tests/modules/migrate_no_migrate_drupal_test/src/Controller/ExecuteMigration.php

@@ -0,0 +1,45 @@
+<?php
+
+namespace Drupal\migrate_no_migrate_drupal_test\Controller;
+
+use Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException;
+use Drupal\Core\Controller\ControllerBase;
+use Drupal\migrate\MigrateExecutable;
+use Drupal\migrate\Plugin\MigrationInterface;
+
+/**
+ * Custom controller to execute the test migrations.
+ *
+ * This controller class is required for the proper functional testing of
+ * migration dependencies. Otherwise, the migration directly executed from the
+ * functional test would use the functional test's class map and autoloader. The
+ * functional test has all the classes available to it but the controller
+ * does not.
+ */
+class ExecuteMigration extends ControllerBase {
+
+  /**
+   * Run the node_migration_no_migrate_drupal test migration.
+   *
+   * @return array
+   *   A renderable array.
+   */
+  public function execute() {
+    $migration_plugin_manager = \Drupal::service('plugin.manager.migration');
+    $definitions = $migration_plugin_manager->getDefinitions();
+    if ($definitions['node_migration_no_migrate_drupal']['label'] !== 'Node Migration No Migrate Drupal') {
+      throw new InvalidPluginDefinitionException('node_migration_no_migrate_drupal');
+    }
+    $migrations = $migration_plugin_manager->createInstances('');
+    $result = (new MigrateExecutable($migrations['node_migration_no_migrate_drupal']))->import();
+    if ($result !== MigrationInterface::RESULT_COMPLETED) {
+      throw new \RuntimeException('Migration failed');
+    }
+
+    return [
+      '#type' => 'markup',
+      '#markup' => 'Migration was successful.',
+    ];
+  }
+
+}

modules/migrate/tests/src/Functional/MigrateNoMigrateDrupalTest.php

@@ -0,0 +1,58 @@
+<?php
+
+namespace Drupal\Tests\migrate\Functional;
+
+use Drupal\node\Entity\Node;
+use Drupal\Tests\BrowserTestBase;
+use Drupal\Tests\node\Traits\ContentTypeCreationTrait;
+
+/**
+ * Execute migration.
+ *
+ * This is intentionally a Functional test instead of a Kernel test because
+ * Kernel tests have proven to not catch all edge cases that are encountered
+ * via a Functional test.
+ *
+ * @group migrate
+ */
+class MigrateNoMigrateDrupalTest extends BrowserTestBase {
+  use ContentTypeCreationTrait;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = [
+    'migrate',
+    'migrate_no_migrate_drupal_test',
+    'node',
+  ];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+    // Ensures that code from the migrate_drupal module can not be autoloaded
+    // while testing on DrupalCI.
+    $this->writeSettings(['settings' => ['deployment_identifier' => (object) ['value' => 'force-new-apcu-key', 'required' => TRUE]]]);
+    $this->createContentType(['type' => 'no_migrate_drupal']);
+  }
+
+  /**
+   * Tests execution of a migration.
+   */
+  public function testExecutionNoMigrateDrupal() {
+    $this->drupalGet('/migrate_no_migrate_drupal_test/execute');
+    $this->assertSession()->pageTextContains('Migration was successful.');
+    $node_1 = Node::load(1);
+    $node_2 = Node::load(2);
+    $this->assertEquals('Node 1', $node_1->label());
+    $this->assertEquals('Node 2', $node_2->label());
+  }
+
+}

modules/migrate_drupal/migrate_drupal.module

@@ -5,12 +5,14 @@
  * Provides migration from other Drupal sites.
  */
 
-use Drupal\Core\Url;
 use Drupal\Core\Database\DatabaseExceptionWrapper;
 use Drupal\Core\Routing\RouteMatchInterface;
+use Drupal\Core\Url;
 use Drupal\migrate\Exception\RequirementsException;
 use Drupal\migrate\MigrateExecutable;
 use Drupal\migrate\Plugin\RequirementsInterface;
+use Drupal\migrate_drupal\MigrationConfigurationTrait;
+use Drupal\migrate_drupal\NodeMigrateType;
 
 /**
  * Implements hook_help().
@@ -28,7 +30,10 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
 /**
  * Implements hook_migration_plugins_alter().
  */
-function migrate_drupal_migration_plugins_alter(&$definitions) {
+function migrate_drupal_migration_plugins_alter(array &$definitions) {
+  $module_handler = \Drupal::service('module_handler');
+  $migration_plugin_manager = \Drupal::service('plugin.manager.migration');
+
   // This is why the deriver can't do this: the 'd6_taxonomy_vocabulary'
   // definition is not available to the deriver as it is running inside
   // getDefinitions().
@@ -45,8 +50,7 @@ function migrate_drupal_migration_plugins_alter(&$definitions) {
         'plugin' => 'null',
       ],
     ];
-    $vocabulary_migration = \Drupal::service('plugin.manager.migration')->createStubMigration($vocabulary_migration_definition);
-    $module_handler = \Drupal::service('module_handler');
+    $vocabulary_migration = $migration_plugin_manager->createStubMigration($vocabulary_migration_definition);
     $translation_active = $module_handler->moduleExists('content_translation');
 
     try {
@@ -91,4 +95,48 @@ function migrate_drupal_migration_plugins_alter(&$definitions) {
       // exist.
     }
   }
+
+  if (!$module_handler->moduleExists('node')) {
+    return;
+  }
+
+  $connection = \Drupal::database();
+  // We need to get the version of the source database in order to check
+  // if the classic or complete node tables have been used in a migration.
+  if (isset($definitions['system_site'])) {
+    // Use the source plugin of the system_site migration to get the
+    // database connection.
+    $migration = $definitions['system_site'];
+    /** @var \Drupal\migrate\Plugin\migrate\source\SqlBase $source_plugin */
+    $source_plugin = $migration_plugin_manager->createStubMigration($migration)
+      ->getSourcePlugin();
+
+    try {
+      $source_connection = $source_plugin->getDatabase();
+      $version = MigrationConfigurationTrait::getLegacyDrupalVersion($source_connection);
+    }
+    catch (\Exception $e) {
+      \Drupal::messenger()
+        ->addError(t('Failed to connect to your database server. The server reports the following message: %error.<ul><li>Is the database server running?</li><li>Does the database exist, and have you entered the correct database name?</li><li>Have you entered the correct username and password?</li><li>Have you entered the correct database hostname?</li></ul>', ['%error' => $e->getMessage()]));
+    }
+  }
+  // If this is a complete node migration then for all migrations, except the
+  // classic node migrations, replace any dependency on a classic node migration
+  // with a dependency on the complete node migration.
+  if (NodeMigrateType::getNodeMigrateType($connection, $version ?? FALSE) === NodeMigrateType::NODE_MIGRATE_TYPE_COMPLETE) {
+    $classic_migration_match = '/d([67])_(node|node_translation|node_revision|node_entity_translation)($|:.*)/';
+    $replace_with_complete_migration = function (&$value, $key, $classic_migration_match) {
+      if (is_string($value)) {
+        $value = preg_replace($classic_migration_match, 'd$1_node_complete$3', $value);
+      }
+    };
+
+    foreach ($definitions as &$definition) {
+      $is_node_classic_migration = preg_match($classic_migration_match, $definition['id']);
+      if (!$is_node_classic_migration && isset($definition['migration_dependencies'])) {
+        array_walk_recursive($definition['migration_dependencies'], $replace_with_complete_migration, $classic_migration_match);
+      }
+    }
+  }
+
 }

modules/node/tests/src/Kernel/Migrate/MigrationPluginAlterTest.php → modules/migrate_drupal/tests/src/Kernel/NodeMigrationTypePluginAlterTest.php

@@ -1,17 +1,17 @@
 <?php
 
-namespace Drupal\Tests\node\Kernel\Migrate;
+namespace Drupal\Tests\migrate_drupal\Kernel;
 
 use Drupal\migrate_drupal\NodeMigrateType;
 use Drupal\Tests\migrate\Kernel\MigrateTestBase;
 use Drupal\Tests\migrate_drupal\Traits\NodeMigrateTypeTestTrait;
 
 /**
- * Tests node_migrations_plugin_alter.
+ * Tests the assignment of the node migration type in migrations_plugin_alter.
  *
- * @group node
+ * @group migrate_drupal
  */
-class MigrationPluginAlterTest extends MigrateTestBase {
+class NodeMigrationTypePluginAlterTest extends MigrateTestBase {
 
   use NodeMigrateTypeTestTrait;
 
@@ -29,7 +29,7 @@ protected function setUp() {
   }
 
   /**
-   * Tests migrate_drupal_migrations_plugin_alter.
+   * Tests the assignment of the node migration type.
    *
    * @param string $type
    *   The type of node migration, 'classic' or 'complete'.
@@ -39,11 +39,12 @@ protected function setUp() {
    *   The expected results.
    *
    * @dataProvider providerMigrationPluginAlter
+   *
+   * @throws \Exception
    */
   public function testMigrationPluginAlter($type, array $migration_definitions, array $expected) {
-    // Version 6 is used so that term node migrations are tested.
     $this->makeNodeMigrateMapTable($type, '7');
-    node_migration_plugins_alter($migration_definitions);
+    migrate_drupal_migration_plugins_alter($migration_definitions);
     $this->assertSame($expected, $migration_definitions);
   }
 

modules/node/node.module

@@ -26,8 +26,6 @@ use Drupal\Core\Url;
 use Drupal\field\Entity\FieldConfig;
 use Drupal\field\Entity\FieldStorageConfig;
 use Drupal\language\ConfigurableLanguageInterface;
-use Drupal\migrate_drupal\MigrationConfigurationTrait;
-use Drupal\migrate_drupal\NodeMigrateType;
 use Drupal\node\Entity\Node;
 use Drupal\node\Entity\NodeType;
 use Drupal\node\NodeInterface;
@@ -1463,52 +1461,3 @@ function node_comment_delete($comment) {
 function node_config_translation_info_alter(&$info) {
   $info['node_type']['class'] = 'Drupal\node\ConfigTranslation\NodeTypeMapper';
 }
-
-/**
- * Implements hook_migration_plugins_alter().
- */
-function node_migration_plugins_alter(array &$definitions) {
-  $version = FALSE;
-  $connection = \Drupal::database();
-  // We need to get the version of the source database in order to check
-  // if the classic or complete node tables have been used in a migration.
-  if (isset($definitions['system_site'])) {
-    // Use the source plugin of the system_site migration to get the
-    // database connection.
-    $migration = $definitions['system_site'];
-    /** @var \Drupal\migrate\Plugin\migrate\source\SqlBase $source_plugin */
-    $source_plugin = \Drupal::service('plugin.manager.migration')
-      ->createStubMigration($migration)
-      ->getSourcePlugin();
-    $source_connection = NULL;
-
-    try {
-      $source_connection = $source_plugin->getDatabase();
-      $version = MigrationConfigurationTrait::getLegacyDrupalVersion($source_connection);
-    }
-    catch (\Exception $e) {
-      \Drupal::messenger()->addError(t('Failed to connect to your database server. The server reports the following message: %error.<ul><li>Is the database server running?</li><li>Does the database exist, and have you entered the correct database name?</li><li>Have you entered the correct username and password?</li><li>Have you entered the correct database hostname?</li></ul>', ['%error' => $e->getMessage()]));
-    }
-  }
-  // If this is a complete node migration then for all migrations, except the
-  // classic node migrations, replace any dependency on a classic node migration
-  // with a dependency on the complete node migration.
-  if (NodeMigrateType::getNodeMigrateType($connection, $version) === NodeMigrateType::NODE_MIGRATE_TYPE_COMPLETE) {
-    // Anonymous function to replace classic node migration plugin IDs with the
-    // node complete plugin ID.
-    $replace_with_complete_migration = function (&$value) {
-      if (is_string($value)) {
-        $value = preg_replace('/d([67])_(node|node_translation|node_revision|node_entity_translation)($|:.*)/', 'd$1_node_complete$3', $value);
-      }
-      return $value;
-    };
-
-    foreach ($definitions as &$definition) {
-      $is_node_classic_migration = preg_match('/d([67])_(node|node_translation|node_revision|node_entity_translation)($|:.*)/', $definition['id']);
-      if (!$is_node_classic_migration && isset($definition['migration_dependencies'])) {
-        array_walk_recursive($definition['migration_dependencies'], $replace_with_complete_migration);
-      }
-    }
-
-  }
-}