Provide clean way to merge configuration into migration plugins

Any work being done on this? It was suggested at https://www.drupal.org/node/2625696#comment-10955205 that this would address the issue of being able to discover only relevant migration plugins. I'm working on getting migrate_tools and migrate_plus working against 8.1.x #2667368: Make migrate_tools work with Drupal 8.1.x and it's not looking good so far...

Another big problem here (besides the run-time destination injection in the issue summary) is the source connection information. The upgrade UI stores source connection key and database info in state keyed as (e.g.) migrate_drupal_6. Previously it set the 'database_state_key' configuration on each source plugin to that key - now it instead sets a global migrate.fallback_state_key. SqlBase::getDatabase() uses this global fallback key for all migrations if there is no explicit database_state_key on the source - even non-Drupal migration, and even if the source plugin contains explicit database configuration.

Previously configuration for migrations was simply part of the configuration entity, now we need a way to save any added/overridden configuration for migration plugins and load it with the plugins. How/where do we do that?

database_state_key is still supported in the general Sql plugin, so general migration can (and should!) use it, but the migrations-as-plugins patch removed the upgrade UI's use of it in favor of that global fallback_state_key.

Looks good! It does runtime plugin configuration injection just how plugins were designed, so... yeah.

I'll hold off on RTBC - it'd be good to get another pair of eyes in case we're missing something obvious. Also, note that whichever of this or #2695297: Refactor EntityFile and use process plugins instead gets in first, the other will need to be rerolled (since both touch where source_base_path is set on upgrades), and I'd like to see that one committed first...

Postponing on #2695297: Refactor EntityFile and use process plugins instead.

EntityFile refactor is in. If I'd known how long it would take that one, I wouldn't have postponed this one, but we can reroll now...

Reroll for 8.2.x with the EntityFile patch. If we still want to commit this to 8.1.x the patch in #8 is the one to go.

I've tested this manually - ran an upgrade through the UI with an HTTP source and the files were properly migrated, demonstrating that the configuration was properly set.

It looks OK to me.

1. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -292,7 +292,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    foreach ($plugin_definition as $key => $value) {
   +    foreach (NestedArray::mergeDeep($plugin_definition, $configuration) as $key => $value) {
   
   +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -77,7 +77,7 @@ protected function getDiscovery() {
   -    $instances = $this->createInstances([$plugin_id], $configuration);
   +    $instances = $this->createInstances([$plugin_id], [$plugin_id => $configuration]);
   

   These fixes looks testable.

2. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
   @@ -103,19 +103,17 @@ public static function run($initial_ids, $operation, $config, &$context) {
   +    if ($definition['destination']['plugin'] === 'entity:file') {
          // Make sure we have a single trailing slash.
   -      $source_base_path = rtrim($config['source_base_path'], '/') . '/';
   -      $source = $migration->getSourceConfiguration();
   -      $source['constants']['source_base_path'] = $source_base_path;
   -      $migration->set('source', $source);
   +      $configuration['source']['constants']['source_base_path'] = rtrim($config['source_base_path'], '/') . '/';
        }
   

   It is a shame we still have to do this special casing. There is an issue about this somewhere.

Test added, let me know if there are any scenarios you'd like added. The interdiff is kinda funky, MigrationPluginManager didn't actually change...

It is a shame we still have to do this special casing. There is an issue about this somewhere.

I don't see one. The idea is that only the migrations with entity:file destinations need the source_base_path - I'm not sure what else we can do here (other than just stuff source_base_path into all migrations). Anyway, I think that's out-of-scope here.

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginConfigurationTest.php
@@ -0,0 +1,74 @@
+      // Add new configuration.

This comment, and the other one in the data provider, are vague. I don't understand what it's talking about. Can this be better documented? Other than that, I don't yet see a problem.

Ah! Better -- now I see what this is trying to do. Assuming the tests pass, and that nobody wants additional test scenarios, I'm comfortable with this change. RTBC.

1. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -292,7 +292,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -    foreach ($plugin_definition as $key => $value) {
   +    foreach (NestedArray::mergeDeep($plugin_definition, $configuration) as $key => $value) {
   

   makes sense

2. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
   @@ -86,7 +86,7 @@ protected function getDiscovery() {
   -    $instances = $this->createInstances([$plugin_id], $configuration);
   +    $instances = $this->createInstances([$plugin_id], [$plugin_id => $configuration]);
   

   is not this a api change?
   configuration passed differently... now

That's not an API change - it's fixing createInstance() to conform to the documented API (FactoryInterface). The $configuration passed to createInstance() is supposed to be simply the plugin's configuration, but without this change that would only worked if the caller had wrapped it in an array keyed by the plugin ID (which createInstance() now does for you, as it should be).

That being said, we should have a change record demonstrating how you can now merge configuration into migration plugins more cleanly...

Draft change record added.

Note that this fix will obviate the need for Migration::set(): #2796755: [PP-1] Deprecate Migration::set()

As an improvement to an experimental module, this is rc eligible per: https://www.drupal.org/core/d8-allowed-changes#rc

Whilst the changes on this issue are an improvement they are not really addressing the fundamental issue that the MigrateUpgradeRunBatch::run() method has to know that it needs to set this configuration. Before proceeding with this patch we need:

• a new title
• issue summary update
• a new issue created to reflect the original purpose of this issue
• an @todo added to that issue in the code

As requested.

Added link to #2804611: Migrate sources and destinations need a way to get their requirements

Committed and pushed 2a3217c to 8.3.x and a707147 to 8.2.x. Thanks!

diff --git a/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginConfigurationTest.php b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginConfigurationTest.php
index 55cce71..827af91 100644
--- a/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginConfigurationTest.php
+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginConfigurationTest.php
@@ -2,9 +2,7 @@
 
 namespace Drupal\Tests\migrate\Kernel\Plugin;
 
-use Drupal\Core\Database\Database;
 use Drupal\KernelTests\KernelTestBase;
-use Drupal\migrate\Plugin\MigrationInterface;
 
 /**
  * Tests the migration plugin manager.

Removed unused uses on commit.

Published change records Configuration may be merged into migration plugins through createInstance()