Migrate Focal Point module from D7 to D9.

I am not able to apply patch. Getting below error:

error: cannot apply binary patch to 'tests/fixtures/files/sites/default/files/2022-03/4thfloor.png' without full index line
error: tests/fixtures/files/sites/default/files/2022-03/4thfloor.png: patch does not apply

By doing initial review of module it seems that more work needs to be done in patch.
Basically focal_point is applied based on settings at /admin/config/media/focal_point. So:

• Widget on drupal 9 will be updated only if Standard image fields is checked
• Same needs to be done for media in case when Media module image fields is checked

I am not able to open patch in Dreditor may be because of size. Why there is so much patch size difference between #4 and #7? Size seems suspicious to me.

Also migration shouldn't be executed if focal_point is not enabled on the source site.

• focal_point_migration.yml: dependencies should be replaced with migration_dependencies.
• MigrationSubscriber.php:Check this condition by enabling only media and doing the migration and then observe for image field.
• FocalPointMigrationTest.php: protected static $configSchemaCheckerExclusions = ['field.field.node.article.field_media'];

Add inheritdoc

Migration looks good to me. Some nits:

1. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,82 @@
   +  public function onPreRowSave(MigratePreRowSaveEvent $event) {
   +
   +    $this->onFieldInstanceWidgetPreRowSave($event);
   

   Extra line.

2. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,82 @@
   +    $result = $event->getMigration()->getSourcePlugin()->getDatabase()->select('variable', 'v')
   +      ->fields('v', ['value'])
   +      ->condition('name', 'focal_point_enabled_for')
   +      ->execute()
   +      ->fetchField();
   +    $focal_point_enabled_for = unserialize($result);
   

   This variable will not be available if module is just enabled and configuration form is not saved. See https://git.drupalcode.org/project/focal_point/-/blob/7.x-1.x/focal_poin.... This needs to be handled here.

3. +++ b/src/Plugin/migrate/process/FocalPointToCropField.php
   @@ -0,0 +1,39 @@
   +/**
   + * Calculates crop_field_Data values.
   + *
   

   crop_field_Data => crop_field_data

4. +++ b/src/Plugin/migrate/source/FocalPoint.php
   @@ -0,0 +1,57 @@
   +  public function prepareRow(Row $row, $keep = TRUE) {
   +    $row->setSourceProperty('fid', $row->getSourceProperty('fid'));
   +    $row->setSourceProperty('focal_point', $row->getSourceProperty('focal_point'));
   +    return parent::prepareRow($row);
   +  }
   

   Seems unnecessary to me.

5. +++ b/tests/fixtures/drupal7.php
   @@ -0,0 +1,23 @@
   +include 'drupal7' . DIRECTORY_SEPARATOR . 'file_usage.php';
   

   Not necessary.

Looks good to me.

The two files used in #14's migration test are about 500kB.

I'm adding a lot smaller patch which is the same as #14 except of these fixture files, but here they consume only ~5.1kB space.

1. +++ b/migrations/focal_point_migration.yml
   @@ -0,0 +1,27 @@
   +id: focal_point_migration
   +label: Focal Point migration
   

   The 'migration' suffix is redundant both in the ID and in the label. Just name it focal_point_crop.

2. +++ b/migrations/focal_point_migration.yml
   @@ -0,0 +1,27 @@
   +  crop_field_data:
   +    plugin: focal_point_to_crop_field
   +    source:
   +      - fid
   +      - focal_point
   

   Since this plugin inspects a destination file, we should pass the destination fid to it, and not the source source fid.

   If you restructure the order of the destination properties here, then you might use the computed @entity_id value (if its process will be changed to migration_lookup).

   Nit: imho the _field suffix in the plugin ID is irrelevant.

3. +++ b/migrations/focal_point_migration.yml
   @@ -0,0 +1,27 @@
   +  entity_id: '@crop_field_data/entity_id'
   

   This value is exactly the same what we pass to the focal_point_to_crop_field as the first source property: so it can be simplified to entity_id: fid.

   But why don't we use a migration lookup instead?

   entity_id:
     plugin: migration_lookup
     source: fid
     migration:
       - d7_file
       - d7_file_private
   

4. +++ b/migrations/focal_point_migration.yml
   @@ -0,0 +1,27 @@
   +  entity_type: '@crop_field_data/entity_type'
   

   This is always file, so it is unnecessary to use a migrate process plugin for providing this value.

   Please specify a constant in the source plugin's configuration, and use that instead!

5. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,85 @@
   +/**
   + * Widget migration event subscriber.
   + */
   

   Would be nice to describe what this event subscriber does.

6. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,85 @@
   +  /**
   +   * Change widget settings to image_focal_point.
   +   */
   

   Changes field widget to focal point for image media entities when needed.

   I also miss parameter comment and the return type (this should be fixed for every other custom method).

7. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,85 @@
   +  /**
   +   * Change widget settings for media.
   +   */
   

   It would be nice to leave a comment here about that this is an optional support for Media Migration module.

8. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,85 @@
   +  protected function onMediaMigrationWidgetPreRowSave(MigratePreRowSaveEvent $event) {
   ...
   +    $focal_point_enabled_for = $this->getFocalPointEnabledFor($event);
   ...
   +    if (!empty($focal_point_enabled_for['media']) && $source_field_widget === 'image_image') {
   ...
   +  protected function onFieldInstanceWidgetPreRowSave(MigratePreRowSaveEvent $event) {
   ...
   +    $focal_point_enabled_for = $this->getFocalPointEnabledFor($event);
   +    if (!empty($focal_point_enabled_for['image'])) {
   ...
   +    $focal_point_enabled_for = unserialize($result);
   +    if (!$focal_point_enabled_for) {
   +      $focal_point_enabled_for['media'] = ['media'];
   +      $focal_point_enabled_for['image'] = ['image'];
   +    }
   

   The focal_point_enabled_for variable isn't available if it wasn't ever saved on the source, but its default value is ['image', 'media']:
   https://git.drupalcode.org/search?search=focal_point_enabled_for&group_i...

   You have to restructure this method, and also change the !empty() checks to in_array().

9. +++ b/src/Plugin/migrate/process/FocalPointToCropField.php
   @@ -0,0 +1,39 @@
   +    $file_storage = \Drupal::entityTypeManager()->getStorage('file');
   

   You should use dependency injection.

10. +++ b/src/Plugin/migrate/process/FocalPointToCropField.php
    @@ -0,0 +1,39 @@
    +    $size = getimagesize($file->getFileUri());
    

    Instead of using getimagesize(), I would use core's ImageFactory to sonstruct an image object, and then get the image with and height by calling $image->getWidth().

    /** @var \Drupal\Core\Image\ImageFactory $image_factory */
    $image = $image_factory->get($file->getFileUri());
    

    ...and then:

    $crop_field_data['x'] = (int) round((($focal_point[0]) / 100) * $image->getWidth());
    

11. +++ b/src/Plugin/migrate/process/FocalPointToCropField.php
    @@ -0,0 +1,39 @@
    +    $crop_type = \Drupal::config('focal_point.settings')->get('crop_type');
    +
    +    $crop_field_data['type'] = $crop_type;
    

    Instead of using the values from the destination site, it would be better to create migrations for the focal_point crop type and even for focal_point.settings.

12. +++ b/src/Plugin/migrate/process/FocalPointToCropField.php
    @@ -0,0 +1,39 @@
    +    $crop_field_data['entity_id'] = $value[0];
    +    $crop_field_data['entity_type'] = 'file';
    

    These should (and can) be done in the migration yaml.

13. +++ b/tests/fixtures/drupal7/users.php
    --- /dev/null
    +++ b/tests/fixtures/drupal7/variable.php
    
    +++ b/tests/fixtures/drupal7/variable.php
    @@ -0,0 +1,47 @@
    +->values(array(
    +  'name' => 'focal_point_enabled_for',
    +  'value' => 'a:2:{s:5:"media";s:5:"media";s:5:"image";i:0;}',
    +))
    

    I don't see any reason why we perform our tests with focal point being disabled for image fields.

14. +++ b/tests/src/Kernel/FocalPointMigrationTest.php
    @@ -0,0 +1,121 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected static $configSchemaCheckerExclusions = ['field.field.node.article.field_media'];
    

    This isn't necessary at all.

15. +++ b/tests/src/Kernel/FocalPointMigrationTest.php
    @@ -0,0 +1,121 @@
    +  /**
    +   * The crop storage.
    +   *
    +   * @var \Drupal\crop\CropStorageInterface
    +   */
    +
    +  protected $cropStorage;
    

    Please remove the empty line between the docblock and the property declaration.

16. +++ b/tests/src/Kernel/FocalPointMigrationTest.php
    @@ -0,0 +1,121 @@
    +    // Two items here (with media migration) are logging migrate messages.
    +    $this->executeMigrations(['d7_field_instance']);
    +    $this->executeMigrations(['d7_field_instance_widget_settings']);
    +    $this->executeMigrations([
    +      'focal_point_migration',
    +    ]);
    

    There are no items there which would display a migration message.

    You can merge these migrations into the first MigrateTestBase::executeMigrations() method call.

17. +++ b/tests/src/Kernel/FocalPointMigrationTest.php
    @@ -0,0 +1,121 @@
    +    $this->assertEquals(645, $image_crop->toArray()['x'][0]['value']);
    +    $this->assertEquals(115, $image_crop->toArray()['y'][0]['value']);
    ...
    +    $this->assertEquals(614, $media_image_crop->toArray()['x'][0]['value']);
    +    $this->assertEquals(100, $media_image_crop->toArray()['y'][0]['value']);
    

    These assertions should be replaced with something which uses Crop API:

    $this->assertEquals(
      ['x' => 645, 'y' => 115],
      $image_crop->position()
    );
    

Re #18:

Imho everything is addressed except #17.6, #17.7 and #17.8.

1. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,84 @@
   +  /**
   +   * Helper function to fetch focal point enabled for.
   +   */
   +  private function getFocalPointEnabledFor(MigratePreRowSaveEvent $event) {
   +    $migration_source = $event->getMigration()->getSourcePlugin();
   +    if (!$migration_source instanceof DrupalSqlBase) {
   +      return;
   +    }
   ...
   +    return $focal_point_enabled_for;
   

   If you return nothing, then the in_array() checks above will fail.

   Pleas clarify what the return type should be here, and update your code accordingly.

2. +++ b/src/EventSubscriber/MigrationSubscriber.php
   @@ -0,0 +1,84 @@
   +    $focal_point_enabled_for = unserialize($result) === FALSE ? [] : unserialize($result);
   +    if (!$focal_point_enabled_for) {
   +      $focal_point_enabled_for += ['media' => 'media'];
   +      $focal_point_enabled_for += ['image' => 'image'];
   +    }
   

   You don't need to check whether $focal_point_enabled_for is empty or not: You can use these default values in the conditional value assignment above.

   And on the other hand, StatementInterface::fetchField() might return with FALSE (when this variable wasn't saved), and unserializing a boolean FALSE makes no sense.

   Just do what DrupalSqlBase::variableGet() does.

   $focal_point_enabled_for = $result !== FALSE
     ? unserialize($result)
     : ['media', 'image'];
   

+++ b/src/EventSubscriber/MigrationSubscriber.php
@@ -0,0 +1,87 @@
+  /**
+   * Changes field widget to focal point for image media entities when needed.
+   */
+  public function onPreRowSave(MigratePreRowSaveEvent $event) {
...
+  /**
+   * Changes field widget settings for media.
+   *
+   * This is an optional support for Media Migration module.
+   */
+  protected function onMediaMigrationWidgetPreRowSave(MigratePreRowSaveEvent $event) {
...
+  /**
+   * Change widget settings for image.
+   */
+  protected function onFieldInstanceWidgetPreRowSave(MigratePreRowSaveEvent $event) {

I'm still missing the parameter documentation from the event subscriber's methods:

/**
 * [Short doc]
 *
 * @param \Drupal\migrate\Event\MigratePreRowSaveEvent $event
 *   Prepare row event.
 */
public function onPreRowSave(MigratePreRowSaveEvent $event) {
  …
}

The patch in #24 addresses every concern we raised.

Definitely RTBC!

@yash.rode, many thanks for your work!

I was able to use your patch to migrate data from D7. Thanks a lot!

Restoring RTBC status for #28, which patch can be applied with cweagans/composer-patches.

Hi. I've noticed that this patch is actually causing an error if you have focal_point module installed without the migrate module. It looks like it ought to check if the module is enabled perhaps in the event subscriber.

PHP Fatal error:  Uncaught Error: Class "Drupal\migrate\Event\MigrateEvents" not found in web/modules/contrib/focal_point/src/EventSubscriber/MigrationSubscriber.php:20

The focal_point_crop migration should have a Content tag instead of Configuration, since it relies on files having been imported.

I might have fixed the MigrateEvents not defined error but I don't know if it's the right way to do that or have even tested it.

Can confirm that patch works. Thanks!

Confirmed again by @vistree at #3393712-3: Update: focal_point migration patch in #3267350 (to prevent error in some scenarios) 👍

Thanks for the patch, I was trying to use the one in comment #31, but in my case I had a problem, because in my source site an image file is missing, then the migration was suddenly interrupted in the middle of the "focal_point_crop" migration, I received the following message:

Error: Call to a member function getFileUri() on null in Drupal\focal_point\Plugin\migrate\process\FocalPointToCropField->transform() (line 77 of /modules/contrib/focal_point/src/Plugin/migrate/process/FocalPointToCropField.php).
[warning] Drush command terminated abnormally.

In case someone has the same problem, I attach a fix for skipping rows where no image file is found.

This is great, I can confirm the patch in comment #34 is working perfectly with the current release without any error and it migrates the focal points with success from Drupal 7!

It would be great to have it committed in a new release!

Thanks to all who worked on this patch!

Can we get the MR for ease of merge, Will merge and release this weekend. Thank you everyone working on this.

MR created from patch in #34

Merged, release to follow shortly.