Migrate Field formatter class third party settings from Drupal 7

1. +++ b/migrations/d7_field_formatter_class.yml
   @@ -0,0 +1,10 @@
   +migration_tags:
   +  - Drupal 7
   

   The 'Configuration' tag has to be added here.

2. +++ b/migrations/d7_field_formatter_class.yml
   @@ -0,0 +1,10 @@
   +process:
   +  field_name: field_name
   

   Now this is completely unused.

3. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,99 @@
   + */
   +class FieldFormatterClassSettings extends DestinationBase implements ContainerFactoryPluginInterface {
   +
   

   Imho this should extend Drupal\migrate\Plugin\migrate\destination\PerComponentEntityDisplay.

   Almost everything is there.

4. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,99 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function import(Row $row, array $old_destination_id_values = []) {
   +    $imported = FALSE;
   ...
   +    }
   +    return $imported;
   +  }
   

   If MigrateDestinationInterface::import() returns booleans, then it means that no mapping is saved. I don't think we want this.

   It is always better to allow the ID map plugin to save a mapping record.

5. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,99 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getIds() {
   +    $ids['id']['type'] = 'integer';
   +    return $ids;
   +  }
   

   This is definitely wrong: this means that every processed Row with the same 'id' represents the same thing.

   (BTW what this 'id' stands for?)

   Check how this looks in Drupal\migrate\Plugin\migrate\destination\ComponentEntityDisplayBase: entity type ID, bundle, field name and view mode.

   (Obviously this is not an issue until the previous above is fixed.)

6. +++ b/src/Plugin/migrate/source/FieldFormatterClassSettings.php
   @@ -0,0 +1,45 @@
   +/**
   + * The field instance per view mode source class.
   + *
   + * @MigrateSource(
   + *   id = "field_formatter_class_settings",
   + *   source_module = "field_formatter_class"
   + * )
   + */
   +class FieldFormatterClassSettings extends DrupalSqlBase {
   +
   

   We don't need this source plugin. The d7_field_instance_per_view_mode in core will unserialize the field instance settings for us,
   so instead of unserializing the data source property in the destination plugin, you will be able to apply proper process(es) in the migration plugin definition.

   We'll also get the view_mode as source property. The field formatter class (if any) will be in formatter/settings/field_formatter_class.

   If we want to be perfect, then we still can extend FieldInstancePerViewMode, then we only have to add a condition to the parent query that explicitly filters for field config instance records having s:21:"field_formatter_class" in their data field, or filter for this setting key in ::initializeIterator().

7. +++ b/tests/fixtures/drupal7.php
   @@ -0,0 +1,45 @@
   +  ->values([
   +    'field_id' => 1,
   +    'field_name' => 'comment_body',
   +    'entity_type' => 'comment',
   +    'bundle' => 'comment_node_page',
   +    'data' => 'a:6:{s:5:"label";s:7:"Comment";s:8:"settings";a:2:{s:15:"text_processing";i:1;s:18:"user_register_form";b:0;}s:8:"required";b:1;s:7:"display";a:1:{s:7:"default";a:5:{s:5:"label";s:6:"hidden";s:4:"type";s:12:"text_default";s:6:"weight";s:1:"0";s:8:"settings";a:1:{s:21:"field_formatter_class";s:9:"classname";}s:6:"module";s:4:"text";}}s:6:"widget";a:4:{s:4:"type";s:13:"text_textarea";s:8:"settings";a:1:{s:4:"rows";i:5;}s:6:"weight";i:0;s:6:"module";s:4:"text";}s:11:"description";s:0:"";}',
   +    'deleted' => 0,
   +  ])
   

   I would like to see a test case with multiple classes, and also an entity type which has at least two fields with field_formatter_settings applied.

Thanks for the review, @huzooka! Back to you, @n4r3n 😊

Great progress, @n4r3n — thanks, and looking forward to that patch with tests 🤓

While we await a more thorough review from @huzooka, here's a superficial one to clean things up:

1. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,36 @@
   +    foreach (array_keys($this->getIds()) as $id) {// echo $id;
   

   I think we want to get rid of the echo $id 🤓

2. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,36 @@
   +    $component['third_party_settings']['field_formatter_class']['class'] =
   

   🤔 This should IMHO use the API instead of an array: setThirdPartySetting().

   See \Drupal\field_group_migrate\Plugin\migrate\destination\FieldGroupEntityViewDisplay for an example.

3. +++ b/tests/fixtures/update/drupal7.php
   @@ -0,0 +1,84 @@
   + * Contains SQL necessary to add a new component for an email field/widget to
   + * the 'node.article.default' entity form display.
   

   🤓 This is still a copy/paste leftover I think :)

4. +++ b/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterClassSettingsTest.php
   @@ -0,0 +1,74 @@
   +  public static $modules = [
   

   🤓 Nit: missing inheritdoc.

5. +++ b/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterClassSettingsTest.php
   @@ -0,0 +1,74 @@
   +    'telephone',
   +    'text',
   +    ];
   

   🤓 Nit: indentation off.

Pretty sure support for using the API for components isn't supported yet, #3015152: Support third-party settings for components within a section

Re #11:

Nice progress! 🙂

My last nits:

1. +++ b/migrations/d7_field_formatter_class.yml
   @@ -0,0 +1,9 @@
   +id: d7_field_formatter_class
   +label: 'Field formatter class settings'
   +migration_tags:
   +  - Drupal 7
   +  - Configuration
   +source:
   +  plugin: field_formatter_class_settings
   +destination:
   +  plugin: field_formatter_class_settings
   

   It's time to use process pipelines:

   (…)
   source:
     plugin: field_formatter_class_settings
   process:
     entity_type: […]
     bundle: […]
     field_name: […]
     view_mode: […]
     field_formatter_class_data: 'formatter/settings/field_formatter_class'
   destination:
     plugin: field_formatter_class_settings
   

   You should replace […] with the corresponding processes from d7_field_formatter_settings in core.

2. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,34 @@
   +    foreach (array_keys($this->getIds()) as $id) {
   +      $values[$id] = $row->get($id);
   +    }
   ...
   +    $component['third_party_settings']['field_formatter_class']['class'] =
   +      $row->get('field_formatter_class_data');
   +    $entity->setComponent($values['field_name'], $component);
   

   Destination plugins should use Row::getDestinationProperty() instead of Row::get().

3. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,37 @@
   +    $component = $entity->getComponent($values['field_name']);
   

   What ensures that this does not return NULL?

4. +++ b/src/Plugin/migrate/source/FieldFormatterClassSettings.php
   @@ -0,0 +1,37 @@
   +        if (!empty($formatter['settings']['field_formatter_class'])) {
   +          $rows[] = array_merge($instance, [
   +            'field_formatter_class_data' => $formatter['settings']['field_formatter_class'],
   +          ]);
   +        }
   ...
   +  }
   

   You can leverage on the parent's FieldInstancePerViewMode::initializeIterator() implementation, and do here something like this:

   /**
    * {@inheritdoc}
    */
   protected function initializeIterator() {
     $instances = parent::initializeIterator();
     $rows = [];
     foreach ($instances->getArrayCopy() as $instance) {
       // Returns only rows with field formatting classes.
       if (!empty($instance['formatter']['settings']['field_formatter_class'])
         $rows[] = $instance;
       }
     }
     return new \ArrayIterator($rows);
   }
   

#13: can you include an interdiff? 🤓 That makes reviewing changes between patches 100x easier! See https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... for how to do that!

Also, can you please in the future change the issue status from Needs work to Needs review when you post a new patch? That's a way to signal to reviewers (i.e. mostly @huzooka here, since he's the primary reviewer — I'm only secondary here) that the patch is ready for the next round of review 😄

1. +++ b/migrations/d7_field_formatter_class.yml
   @@ -6,4 +6,15 @@
   +  bundle:
   +    plugin: static_map
   +    source: bundle
   +    bypass: true
   +    map:
   +      comment_node_forum: comment_forum
   

   This should be like in https://git.drupalcode.org/project/drupal/-/blob/9.1.8/core/modules/fiel...

   bundle:
     -
       plugin: migration_lookup
       migration: d7_field_instance
       source:
         - entity_type
         - bundle
         - field_name
     -
       plugin: extract
       index:
         - 1
   

2. +++ b/migrations/d7_field_formatter_class.yml
   @@ -6,4 +6,15 @@
   +  view_mode: view_mode
   

   See https://git.drupalcode.org/project/drupal/-/blob/9.0.13/core/modules/fie...

   view_mode:
     -
       plugin: migration_lookup
       migration: d7_view_modes
       source:
         - entity_type
         - view_mode
     -
       plugin: extract
       index:
         - 1
     -
       plugin: static_map
       bypass: true
       map:
         full: default
   

3. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -20,12 +20,12 @@
   +    $component = $entity->getComponent($values['field_name']) ?? [];
   

   This makes the originally hidden fields visible. Are you sure we want this?

+++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
@@ -0,0 +1,34 @@
+    $component = $entity->getComponent($values['field_name']) ?? [];

EntityDisplayBase::getComponent() returns NULL in case of hidden components.

In order not to make a hidden field be displayed, we shouldn't migrate field formatter class settings of hidden fields. You should change the line above and add something like this:

+   $component = $entity->getComponent($values['field_name']);
+   if ($component === NULL) {
+     throw new MigrateException(
+       sprintf('Cannot save field formatter class settings for a hidden field %s used in %s %s view mode %s', $values['field_name'], $values['entity_type'], $values['bundle'], $values[static::MODE_NAME]),
+       0, 
+       NULL,
+       MigrationInterface::MESSAGE_INFORMATIONAL,
+       MigrateIdMapInterface::STATUS_IGNORED
+     );
+   }

The code looks good! I also tested the patch in #20 in details, and it is working as expected!

These are my last nits, mostly about comments/annotations – but none of them are crucial!

1. +++ b/src/Plugin/migrate/destination/FieldFormatterClassSettings.php
   @@ -0,0 +1,47 @@
   +  /**
   +   * {@inheritdoc}
   +   * @throws MigrateException
   +   */
   

   The thrown exception should be specified with FQCN (\Drupal\migrate\MigrateException).
   BTW, core has an empty line between {@inheritdoc} and everything that follows it (see Drupal\Core\Config\Entity\ConfigEntityType::checkStorageClass); let's do the same!

2. +++ b/src/Plugin/migrate/source/FieldFormatterClassSettings.php
   @@ -0,0 +1,32 @@
   + * The field instance per view mode source class.
   

   This comment is misleading. Could we have something like "Migration source plugin for field formatter class settings." instead?

3. +++ b/src/Plugin/migrate/source/FieldFormatterClassSettings.php
   @@ -0,0 +1,32 @@
   +class FieldFormatterClassSettings extends FieldInstancePerViewMode {
   

   I think it would be nice to override the source count. Could you please add

   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function doCount() {
   +    return $this->initializeIterator()->count();
   +  }
   +
   

4. +++ b/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterClassSettingsTest.php
   @@ -0,0 +1,82 @@
   + * Migrates various configuration objects owned by the field formatter class module.
   

   Line is longer than 80 chars. Let's replace this with "Tests the migration of Drupal 7 field formatter class settings."

5. +++ b/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterClassSettingsTest.php
   @@ -0,0 +1,82 @@
   + * @group migrate_drupal_7
   

   The test group annotation should use the module's machine name (that's why we cannot see this test executed by DrupalCI imho).

   This should be @group field_formatter_class.

6. +++ b/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterClassSettingsTest.php
   @@ -0,0 +1,82 @@
   +  /**
   +   * Asserts field_formatter_class aspects of a particular component of a view display.
   +   *
   

   Line longer than 80 chars.

7. +++ b/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterClassSettingsTest.php
   @@ -0,0 +1,82 @@
   +    if($component !== null) {
   +      $this->assertIsArray($component);
   +      $this->assertIdentical($class, $component['third_party_settings']['field_formatter_class']['class']);
   +    }
   

   Please remove the condition (which wraps the assertions)!

   It is completely unnecessary and risky.

Great — this is solid now!

Anyone here able to commit the patch in #22 plus #3287508: Automated Drupal 10 compatibility fixes and then tag a new D10 compatible release?