Migrate plugin base classes should implement ConfigurablePluginInterface

+1 for this idea!

+1 on the idea.

Tagging novice. Basically, we'd be doing what is happening over in #2911242-24: Create field collections source migrate plugin. Based on my reading of the related issues, let's introduce this with @TODO's in it to resolve after or with #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface. I'm thinking it makes sense to do this work independently of the related issue, since we can probably move faster in a single module than for the entire plugin system.

It is interesting that we also have to implement calculateDependencies(). I'd be interested in feedback, and if tests pass green.

I see the history of things is that the configurable plugin interface came out of field plugins over in #2271419: Allow field types, widgets, formatters to specify config dependencies. And the calculateDependencies comes from that. I wonder if we need a configurable plugin interface in the plugin system that doesn't require that method?

Here, I've forked the upstream ConfigurablePluginInterface from the plugin system. Because we do not need/want calculateDependencies(). Depending how this fares here, we might want to consider doing that fork upstream too.

I'm going to postpone this on #2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface.

#2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface is in progress, but the decision was to continue to use ConfigurablePluginInterface and require DependentPluginInterface to be implemented separately starting in Drupal 9. As such, we can go ahead with implementing ConfigurablePluginInterface as-is.

This was discussed in the weekly maintainers meeting, so I'm reactivating the issue.

I am queuing up for a test against 8.6.x. When I try to test this locally in phpstorm I get the following errors for all the test data:

Argument 2 passed to Drupal\migrate\ProcessPluginBase::__construct() must be an instance of Drupal\migrate\string, string given, called in /Users/rob/docksal/drupal_core/docroot/core/modules/field/src/Plugin/migrate/process/ProcessField.php on line 85 and defined

I am not sure if this is a local issue which is why I want to run the tests again.

This is due to a bad test ($value shouldn't be passed as the plugin_id in the test because $value could be an array and $plugin_id should be a string) combined with the wrong constructor signature (scalar string should not be in the function definition, breaks older versions of php)

SqlTest also needed to have the setConfiguration method it defined in its test class removed to be compatible with the new interface.

1 on the idea

1. +++ b/core/modules/migrate/src/Plugin/migrate/ConfigurablePluginInterface.php
   @@ -0,0 +1,41 @@
   + * This is very similar to the plugin system's ConfigurablePluginInterface,
   ...
   +interface ConfigurablePluginInterface {
   

   I thought we had discussed using plugin system's ConfigurablePluginInterface. Then we could get rid of this interface.

2. +++ b/core/modules/migrate/src/Plugin/migrate/ConfigurablePluginTrait.php
   @@ -0,0 +1,50 @@
   +trait ConfigurablePluginTrait {
   

   If we use the plugin system's interface, then let's implement calculateDependencies as Final in the trait. So nothing can expand on it and its easy to remove later.

This patch goes back to the core ConfigurablePluginInterface.

I can't implement calculateDependencies as final because it prevents extensions from implementing DependentPluginInterface.

Extensions should be allowed to override calculateDependencies provided they directly implement DependentPluginInterface. Currently \Drupal\migrate\Plugin\migrate\destination\Entity does this.

Why does Entity need to implement calculateDependencies? It isn't a config destination? Seems like a hold over from pre 8.1 when these things were still config.

The configuration needs to preserve the array keys, so made that change here. Also cleaned up doc comments.

This hopefully fixes up some tests. The few I randomly picked returned green in local testing.

Looks like tests are still failing...?!

Ooops. Let's upload the actual patch.

This should fix a bunch more of the errors..

Lets try one more to fix the last test and zap some coding standard issues.

Do we want to rewrite the base plugins to make use of this in this issue, or do we want to get this committed and then refactor them in a separate issue?

The patch here is fairly small and easy to review. It also proves we have a complete set of functionality so let's proceed with it all in a single patch, for now.

1. +++ b/core/modules/migrate/src/Plugin/migrate/ConfigurablePluginTrait.php
   @@ -0,0 +1,59 @@
   +    $this->configuration = NestedArray::mergeDeepArray([$this->defaultConfiguration(), $configuration], TRUE);
   

   Yes, this is perfect.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
   @@ -213,7 +200,7 @@ public function getDestinationModule() {
   +    // which is of the form <module_name>.<configuration object name>.
   

   Stray change, maybe?

I don't see any explicit test of this, so maybe we should add some to this?

Codesniffer wanted a period at the end of the comment, but that hunk doesn't belong to this issue, I've removed it and added unit tests for the trait. I assume we will add/fix more tests when we actually refactor the plugins around this patch.

I like the explicit test coverage. One thing we might want though is a scenario with integer array keys. To test the preserve integer keys of mergeDeepArray. I've added an additional test for that. That is a very important scenario for some of our configurations.

diff --git a/core/modules/migrate/tests/src/Unit/Plugin/migrate/ConfigurablePluginTraitTest.php b/core/modules/migrate/tests/src/Unit/Plugin/migrate/ConfigurablePluginTraitTest.php
new file mode 100644
index 0000000000..d3e6c4b959
--- /dev/null
+++ b/core/modules/migrate/tests/src/Unit/Plugin/migrate/ConfigurablePluginTraitTest.php
@@ -0,0 +1,126 @@
+/**
+ * A test object using ConfigurablePluginTrait.
+ */
+class ConfigurablePluginTestClass {

That should have said "A test class using..."
I fixed that small doc issue that was bugging me, and expanded the testing a bit further to include a couple scenarios with integer keys.

1. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -60,7 +60,7 @@ public function testTransform($method, $value, $expected_value, $migrate_excepti
   +    $this->plugin = new ProcessField(['method' => $method], 'process_field', [], $this->cckFieldManager->reveal(), $this->fieldManager->reveal(), $this->migration->reveal());
   

   I don't understand this change. Is it really related to this patch?

2. +++ b/core/modules/migrate/src/Plugin/migrate/ConfigurablePluginTrait.php
   @@ -0,0 +1,59 @@
   +  /**
   +   * Calculates configuration dependencies for the plugin.
   +   *
   +   * Migration plugins are not configuration, so they have no dependencies.
   +   */
   +  final public function calculateDependencies() {
   +    return [];
   +  }
   

   Should we create a D9 follow-up to remove this and add a todo here?

3. +++ b/core/modules/migrate/tests/src/Unit/Plugin/migrate/ConfigurablePluginTraitTest.php
   @@ -0,0 +1,192 @@
   +        ]
   ...
   +        ]
   +      ]
   

   Nit: Missing commas.

4. +++ b/core/modules/migrate/tests/src/Unit/Plugin/migrate/ConfigurablePluginTraitTest.php
   @@ -0,0 +1,192 @@
   +
   

   Nit: Extra line.

On 1, the test is flawed, and a previous version of this patch brought out the flaw and required fixing the test, but I believe the current version will still pass tests, so I've removed it and will reintroduce it in another issue.

for 2 I added a @todo, but I haven't created an issue, because depending on how #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface and
#2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface play out, it may be removing that method or replacing that whole trait with a new core trait, I'm just not sure yet. I added a base @todo to remove it if/when the interface gets decoupled for now.

I addressed 3 and 4.

The code looks pretty fine to me. Reading the comments I can't say I totally understand why we are removing the calculateDependencies() calls and the IS do not mention this. Maybe we could benefit from a short explanation in the IS as of why this was needed? Marking as NW & Needs IS update for that.

Reviewed in migrate maintainers meeting with @maxocub @mikelutz @heddn. Obviously we need a CR. And I'd also like to see an example conversion of one field, source, process, destination, idmap, migration plugins. We'll add all the remaining "Convert all the things" in a follow-up, but this let's us prove things work.

In this patch we didn't add configurable plugin interface to idmap or field plugins. Neither really use configuration values, and I don't think they need to implement CPI. We did implement it for the migration plugin, but when I went to see about refactoring it, I noticed that in the constructor you have

    foreach (NestedArray::mergeDeep($plugin_definition, $configuration) as $key => $value) {
      $this->$key = $value;
    }

Which just sets anything passed in as a configuration as its own property. I definitely don't like this pattern, but changing it is a bigger refactor of the default migration plugin than I expected.

This patch refactors the Source base plugin and the sql source plugin, the process base plugin and the static map process plugin, and the entity base destination plugin.

Fix those two tests, and break a whole lot more...

If we go through with this migration refactor around configuration, we'll need to document the configuration keys better in the plugin.

Fixing a chunk of tests.

After discussion with @phenaproxima I think we should hold off on implementing CPI in the migration plugin as well. Doing it right requires a greater refactoring which touches a number of issues in the migrate D9 plan. Implementing CPI there should be done in another issue as part of that broader context.

So this patch implements CPI for source, destination, and process plugins.

Added CR

This is a more useful interdiff showing the fixed tests from #46

This looks pretty good. We now have example usage in source, process & destination plugins and all the tests are green. We also have a follow-up to refactor the rest of the migration plugins (#2995393: Refactor migrations plugins that now implement ConfigurablePluginInterface) and we have a change record (https://www.drupal.org/node/3005417).

I only found one nit that held me from RTBCing this patch:

+++ b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
@@ -549,4 +534,15 @@ protected function initializeIterator() {
+  public function defaultConfiguration() {
+    return [
+        'skip_count' => TRUE,
+        'cache_counts' => TRUE,
+        'track_changes' => TRUE,
+      ] + parent::defaultConfiguration();
+  }

Too much indentation.

bah, good catch. There ya go. :-)

+++ b/core/modules/migrate/src/Plugin/migrate/ConfigurablePluginTrait.php
@@ -0,0 +1,63 @@
+    $this->configuration = NestedArray::mergeDeepArray([$this->defaultConfiguration(), $configuration], TRUE);

Nit: we need a comment why we are using merge deep array.

Once the nit is fixed, this is RTBC from me. It's really nice to see this cleaned up.

Comment added.

Nice. Thanks, @mikelutz! Let's get this in front of the committers.

Hi folks, can we get an issue summary update explaining why we're doing our own trait in migrate instead of waiting for #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface - thanks

I've updated the IS, and after a slack discussion with @phenaproxima and @heddn, I've marked the trait as @internal to make it easier to refactor or remove if a core plugin solution exists in the future.

"To facilitate this, create an @internal ConfigurablePluginTrait in the migrate module to use with migration base plugins. There is discussion to do this in the core plugin system in #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface, however for the migration plugins we want to prevent use of the calculateDependencies method from the interface, as these plugins are not configurations and do not use dependencies. Depending on the outcome of that issue and #2946122: Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface there may be a future core plugin library solution to address this, and refactoring the migration plugins around a module trait now will make that process much easier, and provide immediate benefits in cleaning up the plugin code. Making the trait @internal will make sure we can refactor around any potential plugin library solution in the future."

however for the migration plugins we want to prevent use of the calculateDependencies method from the interface, as these plugins are not configurations and do not use dependencies

But plugins live in modules so at least they should provide dependency on their provider, is not it?

But plugins live in modules so at least they should provide dependency on their provider, is not it?

That is not a "calculated dependency" -- that's enforced by the plugin discovery system, which won't find plugins in modules that aren't enabled. Migrate does not need calculateDependencies() and does not use it.

All looks good here. Thanks, @mikelutz! Back to RTBC.

Postponed on #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface per @larowlan

I look into this issue more later, for now I add the tag "Contributed project blocker" because this blocks Feeds Migrate. Feeds Migrate aims to provide an UI for Migrate plugins and therefore it needs Migrate plugins that have configuration be officially declared as configurable.

Triaging issue queue.

#2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface is in, so back to NW

@edmund.dunn, hi, are you still planning to work on this?

Back to postponed since #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface was reverted

This is now unblocked