Problem/Motivation

Field Group configuration migrations shouldn't depend on field formatter migrations. They only require the view mode migrations to be executed.

Proposed resolution

Change the migration requirements of the field group migration plugin definitions and remove the if (!$entity->isNew()) condition from the destination plugins.

Remaining tasks

Patch + test update.

CommentFileSizeAuthor
#2 field_group-migrations_shouldnt_depend_on_field_instance_migration-3177720-2.patch7.68 KBhuzooka

Comments

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Active » Needs review
StatusFileSize
new field_group-migrations_shouldnt_depend_on_field_instance_migration-3177720-2.patch7.68 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Related issues: +#2611904: Add migrate support for D6 fieldgroup

This was introduced in #2611904: Add migrate support for D6 fieldgroup.

  1. -    - d6_field_instance
+    - d6_view_modes
 migration_group: null

…

   plugin: d7_field_group
 migration_dependencies:
   required:
-    - d7_field_formatter_settings
+    - d7_view_modes

    🤔

    This was introduced in #2611904: Add migrate support for D6 fieldgroup with barely any review. I'm not sure I understand yet why we're changing this though, because \Drupal\Core\Entity\Entity\EntityViewMode is not the same as \Drupal\Core\Entity\Entity\EntityViewDisplay.

    And d7_field_formatter_settings would indeed have migrated/created EntityViewDisplay config entities, which is where field_group stores its data.

    So why exactly was this wrong? 

  2. +++ b/contrib/field_group_migrate/src/Plugin/migrate/destination/FieldGroupEntityFormDisplay.php
@@ -25,18 +25,16 @@ class FieldGroupEntityFormDisplay extends PerComponentEntityFormDisplay {
     $entity = $this->getEntity($values['entity_type'], $values['bundle'], $values[static::MODE_NAME]);
-    if (!$entity->isNew()) {
-      $settings = $row->getDestinationProperty('field_group');
-      $settings += [
-        'region' => 'content',
-        'parent_name' => '',
-      ];
-      $entity->setThirdPartySetting('field_group', $row->getDestinationProperty('id'), $settings);
-      if (isset($settings['format_type']) && ($settings['format_type'] == 'no_style' || $settings['format_type'] == 'hidden')) {
-        $entity->unsetThirdPartySetting('field_group', $row->getDestinationProperty('id'));
-      }
-      $entity->save();
+    $settings = $row->getDestinationProperty('field_group');
+    $settings += [
+      'region' => 'content',
+      'parent_name' => '',
+    ];
+    $entity->setThirdPartySetting('field_group', $row->getDestinationProperty('id'), $settings);
+    if (isset($settings['format_type']) && ($settings['format_type'] == 'no_style' || $settings['format_type'] == 'hidden')) {
+      $entity->unsetThirdPartySetting('field_group', $row->getDestinationProperty('id'));
     }
+    $entity->save();
     return array_values($values);

    👍

    Right, this was downright wrong: returning the values of the affected entity view display even if it was not saved, because that destination config entity was not new.

    This would've led to incorrect migration statistics. It would've led to weird behavior.

So: good changes, but AFAICT we've changed the migration dependencies into something incorrect. I feel I'm missing something? 🤓

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Re #3:

I know that \Drupal\Core\Entity\Entity\EntityViewMode is not the same as \Drupal\Core\Entity\Entity\EntityViewDisplay. But there are no (what I know) standalone migration that creates entity form or entity view display entities.

Let's assume that the source Drupal instance has an article node with several fields (including a text field with field name custom_text) which are grouped by field group, even on its form display, and even on its "default" and "teaser" view modes. How are this node type's required configurations migrated?

  1. The field storages (field_storage_config) that are used by this node type (and by other node types, taxonomy terms or users etc) can be migrated very early, because they only depend on the entity type (in this case, this is node).
  2. For being able to migrate the field instance configurations (field_config), you have to migrate node types, because field configurations require a field storage (migrated in step #1 above), the target entity type (node), and the target bundle (article).

    The default entity form – and entity view displays are created by Drupal core when a specific bundle is created. So right after you've the node types migrated, you will also have core.entity_form_display.node.article.default and core.entity_view_display.node.article.default besides node.type.article.

  3. Since node field storages and node types are present, field instance configurations (field_config) can be migrated.
  4. The field widget configuration migrations can be executed at this point, because it only depends on the related field_config entities (migrated by d7_field_instance). In Drupal 7 core, you can't had multiple forms for a node type, that's why there isn't a d7_form_modes migration.
  5. However, field formatter settings can be migrated only when the teaser view mode is also migrated. That's why core's d7_field_formatter_settings depends on the d7_view_modes migrations. This is because entity view displays require a target view mode, and obviously it should be present before the formatter migration is executed.

    So let's execute d7_view_modes as well. This migration will create the view mode core.entity_view_mode.node.teaser view mode, and since it maps the view mode default to full, it will also create a core_entity_view_mode.node.full view mode for nodes.

After that we did the steps above, we have the common base for the pre-existing Field Group migration path and for the "fixed" path I propose here.

Field formatter migrations have a special destination plugin component_entity_view_display. This plugin loads the appropriate entity_view_display entity from the entity display repository; adds the processed field formatter settings to this display entity (or hides the actual component if it should be hidden), and saves it.

At the very first field formatter settings row for the teaser, the loaded entity_view_display will be a new entity: this teaser view display configuration will be created at this point. Every further "field formatter settings row for teaser" row will only modify this view display entity.

But as you can see in this patch (that's why I removed d7_field_instance and d7_field_formatter_settings from the kernel test), the field group settings can be migrated without the d7_field_formatter_settings. The only reason why it was necessary before this change was that the destination plugin checked the existence of the target entity view display entity. If we remove that, the only difference is that the "teaser" view display might be created by Field Group's destination plugin.

Long story short: I'm not saying that the current migration is wrong, I just say that we have unnecessary dependencies (that's why I evaluated this issue as a "task". And I insist that this is true 🙃.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Wow, thanks for this superb write-up! The detail is really helpful 🙏

It all makes perfect sense now, and should also help the field_group maintainer understand why this patch makes sense 👍🥳

  • nils.destoop committed 4f39add on 8.x-3.x authored by huzooka 
    Issue #3177720 by huzooka, Wim Leers: Field Group migrations shouldn't...
nils.destoop’s picture

nils.destoop commented
Status: Reviewed & tested by the community » Fixed

Thx for the clear explanation. I committed the changes to dev.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.