Field formatter mappings defined in getFieldFormatterMap have no effect

Just noting what I found poking at this today.

In d6_field_formatter_settings.yml the input to static_map is a 2 dimensional array but in d7_field_formatter_settings.yml it is a string. However, in CckFieldPluginBase::processFieldFormatter, which adds entries to that static_map, entries are made for the d6 version. Therefor all the static map entries made by processFieldFormatter for d7 are simply ignored. That would explain what hussainweb experienced.

And it is further confused because some of the Cck field plugins use the same getFieldFormatter code for d6 and d7, which makes one think they are the same.

My first thought on a fix is to make the d7 migration like the d6 one. There is also, probably, cleanup to do to make the d7 getFieldFormatters return an empty array.

On the right track?

So, in d7 getFieldFormatterMap adds items to the static map but they will never be accessed. Whether this was done by design or not, it is confusing. A way to change that is modify the migration and add a process plugin. Fortunately, these are similar to the d6 versions, so nothing radical.

In d7_field_formatter_settings.yml the field type is added as the first source to the static map. The new plugin, d7_field_type_defaults, is after the static map. When a formatter is not found in the static map the new plugin will do what the previous version of the static map did, that is return the formatter_type.

No interdiff as this is different approach that doesn't change processFieldFormatter().

Here's an attempt at a review:

Re-using the logic from the d6 migration makes good sense to me.

Renaming the d6 plugin (now that there is a d7 equivalent) maintains a sensible naming convention, but will this renaming have repercussions?

Some of the "options/type" mappings have been removed (e.g. date_default, hidden) - is this intentional? Is there a way to continue handling these? haha, scrub that, I have now seen that they are handled in the plugin.

In the d7 migration template you have retained the existing functionality of skip_on_empty if a value does not map, but the d6 version throws an exception in the plugin - should they both have the same functionality (this may be beyond the scope of this ticket).

Should the plugin name be changed from "field_type_defaults" to "d6_field_type_defaults" in setup() of d6/FieldTypeDefaultsTest.php:
$this->plugin = new FieldTypeDefaults([], 'field_type_defaults', []);
(If this is the case then I'm surprised this test still passes)

+/**
+ * Tests D7 fields defaults.
+ *
+ * @coversDefaultClass \Drupal\field\Plugin\migrate\process\d6\FieldTypeDefaults
+ * @group field
+ */

This should be "d7\FieldTypeDefaults"

Renaming the d6 plugin (now that there is a d7 equivalent) maintains a sensible naming convention, but will this renaming have repercussions?

Yes it will have repercussions existing migrations will not find the plugin:
exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "field_type_defaults" plugin does not exist.' in core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:52

(If this is the case then I'm surprised this test still passes)

I'm also surprised the test passes too, it really shouldn't.

Could we add field_type_defaults as a plugin, leave the test alone(maybe ensure it works as a test). And leave field_type_defaults as a BC layer for d6_field_type_defaults?

Trying my idea out, not sure how to ensure the test is doing what it should but let's see.

Should the tests be also added to D6?

This seems related btw: #2831531: Improved the exception message in d6/FieldTypeDefaults

Self assigning to review this next week.

+++ b/core/modules/field/migration_templates/d7_field_formatter_settings.yml
@@ -51,19 +51,24 @@ process:
-        date_default: datetime_default

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldTypeDefaults.php
@@ -0,0 +1,36 @@
+      if ($row->getSourceProperty('module') == 'date') {
+        $value = 'datetime_default';
+      }

This seems to be dropped from the mapping. Ah, no. It isn't dropped.

But that demonstrates my next thoughts. It would be good to have tests for all of these in D6/D7 to make sure we don't introduce any regressions. A data provider pattern in the test would be a great method to do this.

Patch no longer applies, I'm working on a re-roll and tests.

Here's the re-roll.

This is more complicated and messy than I thought.

In the d6_field_formatter_settings migration:

  "options/type":
      -
        plugin: static_map
        bypass: true
        source:
          - type
          - 'display_settings/format'

The first source of the "options/type" property is "type", the field type, so in the static map the first level of the array is keyed by field types.

But in the processFieldFormatter() method in FieldPluginBase:

    foreach ($this->getFieldFormatterMap() as $source_format => $destination_format) {
      $process[0]['map'][$plugin_id][$source_format] = $destination_format;
    }
    $migration->mergeProcessOfProperty('options/type', $process);

The formatter map is first keyed by the field plugin id.

This work well when the field plugins have the same name as the field type, but it's not always the case. In core, we often have different field plugins for D6 & D7, so they can't have the same ID. Also, one field plugin could be used for different field types (number_integer, number_decimal & number_float). For all these reasons, assuming the field plugin IDs are the same as the field types is not very solid. I think we'll need to fix this.

That fits with my memory of this problem and you have explained it really well.

This is a first try, but I still need to check if all formatter types are covered. Let's see if the tests pass.

I moved all the static_map into the getFieldFormatterMap() method of the different field plugins, so if the tests pass, I think it should proves the the fix works.

Needs review.

Assigning to myself to review later this week.

How do we test that all the field formatter maps are having an effect? Do we need to open up a lot of little issues to test all of them? Anyway, here's some thoughts. Really the first thing is the only thing I found. Except for the worry about how to test all of this.

• +++ b/core/modules/field/src/Plugin/migrate/process/FieldTypeDefaults.php
  @@ -0,0 +1,14 @@
  + * BC Layer.
  + *
  + * @MigrateProcessPlugin(
  + *   id = "field_type_defaults"
  + * )
  + */
  +class FieldTypeDefaults extends D6FieldTypeDefaults {}
  

  We should add @deprecated and trigger_error here as per https://www.drupal.org/core/deprecation.

• +++ b/core/modules/field/migrations/d7_field_formatter_settings.yml
  --- /dev/null
  +++ b/core/modules/field/src/Plugin/migrate/process/FieldTypeDefaults.php
  
  +++ b/core/modules/field/src/Plugin/migrate/process/FieldTypeDefaults.php
  @@ -0,0 +1,14 @@
  +namespace Drupal\field\Plugin\migrate\process;
  

  I don't think we want to move the class out of the d6 namespace if we want to retain BC. Err... actually, what we are retaining here is the plugin id. So that doesn't apply. Good thinking here. We still have the d6 namespaced plugin. And all classes would start using it if they extended things. But we keep the old plugin around in a new namespace. Perfect.

I added the deprecation warning, along with a unit test and a change record.

About your first point regarding how to test all the field formatter maps, I think they are already covered by core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterSettingsTest.php.

I think the fact that this test still passes after moving the static map to the different getFieldFormatterMap() methods proves that they work.

Assuming this comes back green, all feedback is addressed. There's a change record. It's linked into the deprecation. Let's rock and roll.

+++ b/core/lib/Drupal/Core/Field/Plugin/migrate/field/Email.php
@@ -32,10 +32,7 @@ public function getFieldWidgetMap() {
-      'email_formatter_default' => 'basic_string',
-      'email_formatter_contact' => 'basic_string',
-      'email_formatter_plain' => 'basic_string',
-      'email_formatter_spamspan' => 'basic_string',

Oups, this should not have been removed. This plugin is used by both D6 & D7 and these formatter types are in the D6 Email module.

This means that this is not tested. These formatter types are not present in the D6 fixture so we can't easily test them now.

I propose that we open one follow-up to check that *all* formatter types from Drupal 6 & 7 are mapped and tested. We should also open one for the widget types.

Follow-ups created:

• #2944604: [META] Check that all field formatter types are mapped and tested
• #2944605: [META] Check that all field widget types are mapped and tested

Can we update the issue summary? I'm ok with the follow us. Let's make it clear though in this issue what we are doing. And what the follow ups will accomplish.

IS updated.

Further IS improvements.

Thanks for such a nice IS update. One question, does it make sense to make d6 the same as d7 and move all of its static map changes into getFieldFormatterMap() as well? If we think we should, we can open that up as a follow-up or add it to the scope of the existing follow-ups.

It would be great if we move the d6 static map to the field plugins and clean it up.

Just looking at the email field type we can see inconsistency.

From the d6_field_formatter_settings:

  default: email_mailto
  spamspan: email_mailto
  contact: email_mailto
  plain: basic_string

And from the Email field plugin:

  'email_formatter_default' => 'basic_string',
  'email_formatter_contact' => 'basic_string',
  'email_formatter_plain' => 'basic_string',
  'email_formatter_spamspan' => 'basic_string',

The formatter types are not the same and the mappings are different.

But this is out of scope here and I think we should add this to the scope of the follow-ups (Done).

Issue summary is updated. We've punted some things to follow-ups. A draft CR was added. I think we are good again.

+++ b/core/modules/field/src/Plugin/migrate/process/FieldTypeDefaults.php
@@ -0,0 +1,21 @@
+ * Use d6_field_type_defaults of d7_field_type_defaults instead.

typo:s/of/or - fixed on commit.

This looks OK to backport to 8.5.x, so moving there and 'to be ported'.

Committed b89d3e4 and pushed to 8.6.x. Thanks!

Discussed on the core committer call; it seems advantageous to commit this to 8.5.x given we are doing a big push around how Migrate is ready for site builders.

Of course #34 won't apply in 8.6. Can we post a patch against 8.5 here so it can sit in the rtbc queue? Tagging novice.

Committed 594a0f6 and pushed to 8.5.x. Thanks!

Discussed with @catch and we felt this should be in 8.5.0.