MigrationPluginManager::getDefinitions() blows up in node derivers

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -127,7 +135,29 @@ protected function setUpDatabase(array $database_info) {
   +      if ($key == 'migrate') {
   

   What if someone for legacy reasons defined their connection 'migrate'? Why does it matter if the key exists or not? Just add the key to the requirements exception and throw it anyway.

2. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
   @@ -107,7 +117,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   -      foreach (static::getSourcePlugin('d6_node_type') as $row) {
   +      foreach ($node_types as $row) {
   

   Is this necessary for fixing this error?

3. +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
   @@ -84,7 +94,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   -      foreach (static::getSourcePlugin('d7_node_type') as $row) {
   +      foreach ($node_types as $row) {
   

   Same here. This seems like a stray code cleanup.

4. +++ b/core/modules/node/tests/src/Kernel/Migrate/MigratePluginListTest.php
   @@ -0,0 +1,57 @@
   +    unset($migration_plugins['user_picture_field']);
   

   Can we check migration plugins that use a source plugin of embed_data? That would make this more extensible for the future.

5. +++ b/core/modules/node/tests/src/Kernel/Migrate/MigratePluginListTest.php
   @@ -0,0 +1,57 @@
   +    unset($migration_plugins['user_picture_field']);
   

   Can we check migration plugins that use a source plugin of embed_data?

+++ b/core/modules/node/tests/src/Kernel/Migrate/MigratePluginListTest.php
@@ -49,9 +49,11 @@
+      $this->assertNotInstanceOf('Drupal\migrate\Plugin\migrate\source\SqlBase',

Nit:
\Drupal\migrate\Plugin\migrate\source\SqlBase::class is more resilient. Or even import that thing and SqlBase::class

Self-assigning for review.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -127,12 +130,17 @@ public function getDatabase() {
   +   *   Thrown if there is no properly-configured database.
   

   Can this be changed to "Thrown if no source database connection is configured"?

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -127,12 +130,17 @@ public function getDatabase() {
   +      // If there is no explicit database configuration at all, fallback to a
   

   Nit: Should be "fall back".

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -144,7 +152,29 @@ protected function setUpDatabase(array $database_info) {
   +        throw new RequirementsException("No database connection configured for source plugin " . $this->pluginId);
   

   Let's pass the original ConnectionNotDefinedException to the RequirementsException as the previous exception (the fourth constructor argument).

4. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -144,7 +152,29 @@ protected function setUpDatabase(array $database_info) {
   +    if ($this->pluginDefinition['requirements_met'] === TRUE) {
   

   What does requirements_met do? Is it documented in the plugin annotation? If not, can this be commented?

5. +++ b/core/modules/node/tests/src/Kernel/Migrate/MigratePluginListTest.php
   @@ -0,0 +1,59 @@
   +namespace Drupal\Tests\node\Kernel\Migrate;
   

   Why is this test part of Node? Seems like it should be part of Migrate.

6. +++ b/core/modules/node/tests/src/Kernel/Migrate/MigratePluginListTest.php
   @@ -0,0 +1,59 @@
   +    // Any database-based source plugins should fail a requirements test in the
   +    // absence of a database connection.
   

   Let's clarify this a bit: "...in the absence of a source database connection (i.e., a connection with the 'migrate' key)."

7. +++ b/core/modules/node/tests/src/Kernel/Migrate/MigratePluginListTest.php
   @@ -0,0 +1,59 @@
   +      if ($migration->getSourcePlugin() instanceof RequirementsInterface) {
   

   This is kind of a nit, but I don't really like that we're calling $migration->getSourcePlugin() three times, especially since we have no need for the rest of the migration. Can we do something like array_map(function ($migration_plugin) { return $migration_plugin->getSourcePlugin(); }, $migration_plugins) to avoid this?

Self-assigning for review...again.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
@@ -144,7 +152,29 @@ protected function setUpDatabase(array $database_info) {
+        throw new RequirementsException("No database connection configured for source plugin " . $this->pluginId,[], 0, $e);

Supernit: there should be a space after $this->pluginId. Fixable on commit.

Otherwise, I love it.

Setting this issue to Needs Review. @phenaproxima I am uploading the new patch which follows the suggestion in #26.

I am setting this to RTBC as set by @phenaproxima.

Applying this patch improved my luck in attempting to import via wordpress_migrate , see #2839482: Wordpress Migration UI dies with a ConnectionNotDefinedException when import starts for the gory details. I got my posts into the site correctly, if not the categories! RTBC please.

The fix looks fine but is there somewhere we can add some docs for contrib doing similar with derivers?

working on that.

Add a CR.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x, thanks!

Thank you catch, mikeryan and everyone - I appreciate the prompt attention to this issue. Best regards.

Published change record.

Publish change record.