Create a ProcessField plugin to process the field types

Applied the patch to my local build and stepped through MigrateFieldTest.php as the prime example of how this should work. The new plugin runs along the established lines of the "field_type" plugin and works as expected.

The changes added in #5 makes sense to speed up the acceptance of this patch.

+++ b/core/modules/field/migration_templates/d7_field.yml
new file mode 100644
index 0000000..9c7571b

index 0000000..9c7571b
--- /dev/null

--- /dev/null
+++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php

+++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
+++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
@@ -0,0 +1,140 @@

@@ -0,0 +1,140 @@
+<?php
+

There's no explicit test coverage for this, shouldn't there be some?

All depends on what's going to rely on the deprecated code, which I'm not entirely sure about at this point tbh. I'd lean towards leaving it in unless it's already effectively dead code.

Just committed #2901851: Replace the static map in the d7_field migration by field plugins by the way but afraid this means another re-roll here.

As mentioned in #12, a re-roll was required.

Test failures are unrelated to this issue, they occur even when the patch is not applied (which is concerning...)

@cilefen, @effulgentsia, @Cottser and I discussed this and agreed it makes sense to handle it as a critical as a blocker for #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted). While the title doesn't really describe a critical issue on its own, and we don't typically hard-block criticals on API additions, it's helpful here because (as @catch mentioned) we lost track of an earlier issue in the chain and the rest have gone quickly once we were able to promote them for visibility. And, as @effulgentsia mentioned, there isn't really a good solution for this currently, so there's not some hotfix we should pursue instead.

Thanks for the ongoing work on this chain of issues!

This is an excellent patch. First-class work all around! It wouldn't be me if I didn't raise a few questions, though :)

1. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -0,0 +1,140 @@
   + * If no field plugin for the given field type is found, the source field type
   + * will be returned back to the process pipeline.
   

   I think we should probably return NULL instead. So that, say, one could skip_on_empty. Either that, or throw an exception? If no appropriate field plugin is found, I think it's safer to consider that an uncertain condition that requires developer intervention.

2. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -0,0 +1,140 @@
   +   * The migration object.
   

   Nit: Should match the constructor parameter.

3. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -0,0 +1,140 @@
   +    $field_type = is_array($value) ? $value[0] : $value;
   

   Let's change $value[0] to reset($value), just in case $value is associative.

4. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -0,0 +1,140 @@
   +      $plugin_id = $this->fieldPluginManager->getPluginIdFromFieldType($field_type, [], $this->migration);
   +      $plugin_instance = $this->fieldPluginManager->createInstance($plugin_id, [], $this->migration);
   +      if (!method_exists($plugin_instance, $method) || !is_callable([$plugin_instance, $method])) {
   +        throw new MigrateException('The specified method does not exists or is not callable.');
   +      }
   +      return call_user_func_array([$plugin_instance, $method], [$row]);
   

   Can we abstract this a bit as a helper method that accepts the plugin manager to try, rather than repeat the code entirely?

5. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -0,0 +1,120 @@
   + * @group legacy
   

   Interesting, I've never seen this test group before in core. Is it a new thing?

6. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -0,0 +1,120 @@
   +class ProcessFieldTest extends MigrateProcessTestCase {
   

   I think we should refactor this test class to use the data provider pattern, which would make it easier for us to test new methods, new combinations and types of arguments, etc.

7. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -0,0 +1,120 @@
   +    $this->cckFieldManager = $this->getMock(MigrateCckFieldPluginManagerInterface::class);
   +    $this->fieldInstance = $this->getMock(MigrateFieldInterface::class);
   +    $this->fieldManager = $this->getMock(MigrateFieldPluginManagerInterface::class);
   +    $this->migration = $this->getMock(MigrationInterface::class);
   

   I would prefer if we used Prophecy for all this, but I'm not married to it.

   Also, for clarity, can $this->fieldInstance be renamed to $this->fieldPlugin?

On #5 it allows the test to pass without warnings despite it using deprecated code. https://www.drupal.org/core/deprecation

Moving back to 8.4.x since it's eligible.

All the karma unto @maxocub. You are a rock star.

A few more things jumped out at me, but they should all be reasonably easy fixes. They basically come down to documentation.

1. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -115,26 +114,37 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +  /**
   +   * Instantiate a field plugin and call a method on it.
   +   *
   +   * @param \Drupal\migrate_drupal\Plugin\MigrateFieldPluginManagerInterface $field_plugin_manager
   +   *   The field plugin manager.
   +   * @param string $field_type
   +   *   The field type for which to get the field plugin.
   +   * @param string $method
   +   *   The method to call on the field plugin.
   +   * @param \Drupal\migrate\Row $row
   +   *   The row from the source to process.
   +   */
   +  protected function callMethodOnFieldPlugin(MigrateFieldPluginManagerInterface $field_plugin_manager, $field_type, $method, Row $row) {
   

   Missing @return documentation.

2. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -115,26 +114,37 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    if (!method_exists($plugin_instance, $method) || !is_callable([$plugin_instance, $method])) {
   

   Is the method_exists() call necessary? Won't is_callable() do essentially the same thing?

1. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -0,0 +1,118 @@
   +    if ($migrate_exception) {
   +      $this->setExpectedException(MigrateException::class);
   +    }
   

   Ideally, $migrate_exception would be the full or partial text of the exception message, and we would assert that. ($this->setExpectedExceptionMessage() or something)

2. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -0,0 +1,118 @@
   +    return [
   +      [
   +        'getFieldType',
   +        'foo',
   +        'bar',
   +        FALSE,
   +        FALSE,
   +      ],
   +      [
   +        'getFieldFormatterMap',
   +        'foo',
   +        ['foo' => 'bar'],
   +        FALSE,
   +        FALSE,
   +      ],
   +      [
   +        'getFieldWidgetMap',
   +        'foo',
   +        ['foo' => 'bar'],
   +        FALSE,
   +        FALSE,
   +      ],
   +      [
   +        '',
   +        '',
   +        '',
   +        TRUE,
   +        FALSE,
   +      ],
   +      [
   +        'getFieldType',
   +        'foo',
   +        NULL,
   +        FALSE,
   +        TRUE,
   +      ],
   +    ];
   

   Can you add some comments here to briefly explain each scenario we're running, what we are doing in each, and what we expect to happen? I <3 the data provider pattern, but it can sometimes obfuscate our intentions.

This looks beautiful and should pass Drupal CI. RTBC once it does.

A couple questions/comments.

1. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -0,0 +1,153 @@
   +    $field_type = is_array($value) ? reset($value) : $value;
   

   In what cases could this be an array? Are we assuming the first array value is the field_type? That seems like magic and bad.

   I also don't see a test for this scenario, so that maybe would clear up what we're trying to do here.

2. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -0,0 +1,123 @@
   +        'getFieldType',
   +        'foo',
   +        'bar',
   +        FALSE,
   +        FALSE,
   

   It would be helpful if the keys on this were the variable names being passed into the test function.

1. +++ b/core/modules/field/src/Plugin/migrate/process/ProcessField.php
   @@ -106,7 +106,9 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if (!is_string($value)) {
   

   I think this should be is_scalar. See https://3v4l.org/lVPqC

2. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/ProcessFieldTest.php
   @@ -56,7 +56,7 @@ protected function setUp() {
   +  public function testTransform($method, $field_type, $expected_value, $migrate_exception = '', $plugin_not_found = FALSE) {
   

   This should be $value instead of $field_type, no?

All feedback addressed. Back to RTBC if tests pass.

Committed as bd2227e and pushed to 8.5.x.

Cherry-picked as 63bd90d and pushed to 8.4.x

Great work!

Oh - can we get a change record here?

published that

I don't think we need to mention this issue specifically, just the one that it unblocked which is the big win. Thanks all!