Fix bugs of D7->D9 insert settings migration

Since the module is in a beta phase, we cannot remove the migrate plugins; but since they also contains bugs, I will include them in this review.

Preexisting plugins:

1. PerComponentEntityFormDisplayInsert destination plugin:
   

   // Add Insert module third party settings to field settings:
   $thirdPartySettings = $row->getDestinationProperty('options/third_party_settings');
   if (count($thirdPartySettings['insert'])) {
     $options = $entity->getComponent($values['field_name']);
     $options['third_party_settings']['insert'] = $thirdPartySettings['insert'];
     $entity->setComponent($values['field_name'], $options);
   }
   

   I see two issues here:

   1. Since the FieldInstanceWidgetInsertSettings process plugin might return with an empty array as well, the insert key in $thirdPartySettings might be completely missing. This means that count($thirdPartySettings['insert']) will throw an error.

      Solution: instead of getting the value of 'options/third_party_settings', we should read the 'options/third_party_settings/insert' destination property. If it is NULL, then we don't have to save anything.

   2. Fields with insert configs might be hidden as well. For hidden form components, EntityDisplayInterface::getComponent() will return with NULL. This also triggers an error.

      Solution: if $entity->getComponent($values['field_name']); returns with NULL, then the field is hidden, so we can't (and shouldn't) save the third party settings.

   The best (compatible) solution is for #1 is instead of getting the value of 'options/third_party_settings', we should read the 'options/third_party_settings/insert' destination property. If it is NULL, then we don't have to save anything.

   // Add Insert module third party settings to field settings:
   if (
     $insert_settings = $row->getDestinationProperty('options/third_party_settings/insert') &&
     $field_component = $entity->getComponent($values['field_name'])
   ) {
     $field_component['third_party_settings']['insert'] = $insert_settings;
     $entity->setComponent($values['field_name'], $field_component);
   }
   

2. FieldInstanceWidgetInsertSettings process plugin:
   

   // While Insert features a dedicated "enabled" checkbox
   // ($widget_settings['insert']) in D7, Insert is enabled whenever one or
   // more styles are activated in D8. Therefore, if Insert is disabled in D7,
   // deactivate all styles in D8.
   if ($widget_settings['insert']) {
     foreach ($widget_settings['insert_styles'] as $style) {
       $style = preg_replace('/^image_/', '', $style);
       $styles[$style] = $style;
     }
   }
   
   return [
     'insert' => [
       'styles' => $styles,
       'default' => $widget_settings['insert_default'],
       'class' => $widget_settings['insert_class'],
       'width' => $widget_settings['insert_width'],
     ],
   ];
   

   1. Here, the 'insert_style' mapping does not maps Drupal 7 auto to Drupal 9 insert__auto, and isn't aware of that the insert_default might also contain an image_ prefixed image style ID.

      Suggested solution: add a helper function which does the key mapping, and use it in the foreach loop and also for the default style:

      public static function getTargetStyleFromSource($source_style = NULL): string {
        // Map 'auto' to 'insert__auto'.
        if ($source_style === 'auto') {
          return 'insert__auto';
        }
        // Map D7 image styles to D9 image style config name.
        if (preg_match('/^image_(.*)$/', $source_style, $matches)) {
          return $matches[1];
        }
      
        return $source_style ?? INSERT_DEFAULT_SETTINGS['default'];
      }
      

   2. But the bigger problem is that it only uses the value from the insert_style array, so we might try to process 0 values instead of the actual keys like link, image etc.

      Solution: process the array keys instead, and use that as the key value as well if the value is not empty

      // While Insert features a dedicated "enabled" checkbox
      // ($widget_settings['insert']) in D7, Insert is enabled whenever one or
      // more styles are activated in D8. Therefore, if Insert is disabled in D7,
      // deactivate all styles in D8.
      if ($widget_settings['insert'] && $widget_settings['insert_styles']) {
        foreach ($widget_settings['insert_styles'] as $style_key => $style_value) {
          $style_key = static::getTargetStyleFromSource($style_key);
          $insert_settings['styles'][$style_key] = $style_value ? $style_key : 0;
        }
      }
      

Review of patch in #2:

The row prepare hook produces different Insert third party settings than the preexisting migration + the preexisting process and destination plugins: it doesn't removes the image_ prefix from the image style specific insert styles, it uses a wrong key style instead of styles, and it migrates these style mapping even when it shouldn't do so (for the field_cmnt_image and field_file field formatters in the gallery node type's default form).

I suggest starting over:

1. Keep your test fixture and kernel test!
2. Fix the preexisting process and destination plugins, and then
3. In the migration test, execute the preexisting d7_field_instance_widget_insert_settings migration, and change your assertions according the actual results.
4. After you're done with the above, remove d7_field_instance_widget_insert_settings from your migration test, and return to the migrate prepare row hook implementation. You should produce the same results what you had in point 3.

Later on we also have to create an update hook for being able to remove the migration yaml file (without a BC break), but for first, let's focus on these!

I'm uploading a "dummy" schema yaml patch (hoping that it will help not just your work, but also the maintainer).

1. +++ b/insert.module
   @@ -1036,3 +1040,87 @@ function insert_help($route_name) {
   +  $supported_field_types = [
   +    'image',
   +    'file',
   +  ];
   +  if (!in_array($row->getSourceProperty('type'), $supported_field_types, TRUE)) {
   +    return;
   +  }
   +  $entity_type_list = [
   +    'node',
   +    'comment',
   +  ];
   +  if (!in_array($row->getSourceProperty('entity_type'), $entity_type_list, TRUE)) {
   +    return;
   +  }
   

   We don't need these restrictions.

2. +++ b/insert.module
   @@ -1036,3 +1040,87 @@ function insert_help($route_name) {
   +  if (!$source instanceof DrupalSqlBase) {
   +    return;
   +  }
   +  $database_connection = $source->getDatabase();
   +  $result = $database_connection->select('field_config_instance', 'fci')
   +    ->fields('fci', ['data'])
   +    ->condition('fci.field_name', $row->getSourceProperty('field_name'))
   +    ->condition('fci.bundle', $row->getSourceProperty('bundle'))
   +    ->execute()
   +    ->fetchField();
   +  $insert_settings = $result !== FALSE ? unserialize($result) : FALSE;
   

   We don't need this query: you try to get data what is already available in the current row.

3. +++ b/insert.module
   @@ -1036,3 +1040,87 @@ function insert_help($route_name) {
   +        'default' => $insert_settings['widget']['settings']['insert_default'],
   
   +++ b/src/Plugin/migrate/process/FieldInstanceWidgetInsertSettings.php
   @@ -37,22 +37,20 @@ class FieldInstanceWidgetInsertSettings extends ProcessPluginBase {
            'default' => $widget_settings['insert_default'],
   

   Default style also should be mapped.

4. @srishti.bankar, I checked why you don't migrate any insert settings for field_file or field_cmnt_image, and actually, you're right, and the preexisting migrate process plugin still has a bug:
   

   public function getInsertSettings(array $widget_settings) {
     if (!isset($widget_settings['insert'])) {
       return [];
     }
   

   That !isset($widget_settings['insert']) should be replaced with empty($widget_settings['insert']). Why? Because if this settings does not exist, or it is set to 0, the insert button wasn't shown on the source site.

5. I think that we should outsource the logic into a new a class which will parse the Drupal 7 widget settings and return the right Drupal 9 insert third party settings. If we do so, we don't want to maintain the same logic in two files.

@srishti.bankar, great work 🥳! Imho #6 is functionally perfect, I only have some very-very little nits:

1. +++ b/src/Plugin/migrate/destination/PerComponentEntityFormDisplayInsert.php
   @@ -26,12 +26,14 @@ class PerComponentEntityFormDisplayInsert extends PerComponentEntityFormDisplay
   +    $insert_settings &&
   +    $field_component = $entity->getComponent($values['field_name'])
   

   Nit: indentation error.

2. +++ b/src/Plugin/migrate/process/FieldInstanceWidgetInsertSettings.php
   index 0000000..7297420
   --- /dev/null
   
   --- /dev/null
   +++ b/src/Utility/InsertWidgetSettings.php
   
   +++ b/src/Utility/InsertWidgetSettings.php
   +++ b/src/Utility/InsertWidgetSettings.php
   @@ -0,0 +1,48 @@
   
   @@ -0,0 +1,48 @@
   +<?php
   +
   +namespace Drupal\insert\Utility;
   +
   +/**
   + *
   + */
   +class InsertWidgetSettings {
   

   Nit: it would be better to rename this make it clear that it is a migration utility. I would add Migrate (either as prefix, or as a suffix). I also miss the class/method comments.

3. +++ b/src/Utility/InsertWidgetSettings.php
   @@ -0,0 +1,48 @@
   +  public static function getInsertWidgetSettings(array $settings) {
   +    if (!array_key_exists('insert', $settings)) {
   +      return NULL;
   +    }
   +    if ($settings['insert'] && $settings['insert_styles']) {
   

   It would be better to check whether $settings['insert] is empty. Because you should return NULL even if it is '0'.

   If you do so, then you can remove the condition in the next line, and just process the widget settings you have.

4. +++ b/tests/src/Kernel/InsertMigrateTest.php
   @@ -0,0 +1,117 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    // @todo Change the autogenerated stub
   

   Nit: could you please remove this todo comment?

5. +++ b/tests/src/Kernel/InsertMigrateTest.php
   @@ -0,0 +1,117 @@
   +  /**
   +   * Tests insert settings migration.
   +   */
   +  public function testInsertMigration(): void {
   ...
   +    $this->assertNoMigrationMessages();
   ...
   +  /**
   +   * DX.
   +   */
   +  public function assertNoMigrationMessages() {
   

   Nit: could you replace this $this->assertNoMigrationMessages(); with a non-custom assertion? I know this just checks whether no migration messages were logged during the test, and those messages are stored in the protected $migrationMessages property, so $this->assertEmpty($this->migrateMessages); would be fine.

I think I also have a plan how to deprecate the preexisting migration. If the nits above are addressed, I will implement it.

#8 addresses all of my concerns.

Let's deprecate the standalone migration!

We still need a change record, but for now, let's try to get an approval from the maintainers!

Excuse me for not checking sooner, I have not been around for a while. I can see you have spent a lot of time on making a proper patch! That's great, very appreciated! I have looked at the code and would certainly be fine merging it. While I do not have time to test it properly, I can see you have been working on it together which I guess qualifies for reviewed and tested by the community.

I'm sorry I don't manage finding sufficient time to properly maintain the project as of now. If there is something left to do on the change, feel free to add. In any case, I'd be super ok merging. I have seen the notes on deprecating migrations to be removed in a future beta version 3.0. I'm just not 100% sure which code is to be removed then, in addition to deleting PerComponentEntityFormDisplayInsert and FieldInstanceWidgetInsertSettings. I guess I would then also remove dealing with insert_migration_plugins_alter in insert_migration_plugins_alter and remove MigrateInsertWidgetSettings::standaloneMigrationIsOmittable as well as d7_field_instance_widget_insert_settings.yml along?

Just a tiny thing my IDE was complaining when looking at the code:

protected static function getTargetStyleFromSource($source_style = NULL): string {

in MigrateInsertWidgetSettings would be missing the parameter's type declaration:

protected static function getTargetStyleFromSource(string $source_style = NULL): string {

I'll just add that before merging in, if there is nothing else left.

Sorry again for not being around for a long time.