Upgrade path for Image 7.x

Found a few thangs...

1. +++ b/core/modules/image/migration_templates/d7_image_styles.yml
   @@ -0,0 +1,20 @@
   +  name:
   +    -
   +      plugin: machine_name
   +      source: name
   +    -
   +      plugin: dedupe_entity
   +      entity_type: image_style
   +      field: name
   +      length: 32
   

   Let's lose the machine_name and dedupe_entity plugins here -- we want to merge with existing styles by machine name, so this can just be a direct mapping (name: name).

2. +++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
   @@ -0,0 +1,74 @@
   + * @MigrateSource(
   + *   id = "d7_image_styles"
   + * )
   

   As @lostkangaroo said, please do add source_provider = "image".

3. +++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
   @@ -0,0 +1,74 @@
   +    return $this->select('image_styles', 'ims')
   +      ->fields('ims');
   

   'ims' is a strange alias for the table. Can it just be 'is'?

4. +++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
   @@ -0,0 +1,74 @@
   +    foreach($results as $key => $result) {
   

   Need a space between foreach and the opening parenthesis.

5. +++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
   @@ -0,0 +1,74 @@
   +        'id'     => $result['name'],
   

   'name' should not be changed to 'id'. The source plugins should stay as close as possible to the original data.

6. +++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
   @@ -0,0 +1,74 @@
   +        'data'   => $result['data'],
   +      ];
   +      $effects[$key]['data'] = unserialize($result['data']);
   

   Why not simply inline the call to unserialize()? 'data' => unserialize($result['data'])

Oh, and. :)

The plugin won't import the image style effects with key 'name' instead of 'id'. How would I go about fixing that then?

You will need to change it in the process pipeline, within the migration entity itself.

+process:
+  name: name
+  label: label
+  effects: effects

This should be:

+process:
+  id: name
+  label: label
+  effects: effects

Other, minor things -- once these are fixed and DrupalCI approves, it's RTBC from me.

1. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageStylesTest.php
   @@ -0,0 +1,76 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   Please change this to {@inheritdoc} -- we're going to do that in another issue for all existing tests, so best to make it consistent here :)

2. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageStylesTest.php
   @@ -0,0 +1,76 @@
   +   * Asserts various aspects of a ImageStyle entity.
   

   The nit to end all nits: s/a/an

3. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageStylesTest.php
   @@ -0,0 +1,76 @@
   +   * @param string $id
   +   *   The expected image style ID.
   +   * @param string $label
   +   *   The expected image style label.
   

   Missing documentation for the other parameters.

4. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageStylesTest.php
   @@ -0,0 +1,76 @@
   +  protected function assertEntity($id, $label, $expected_size, $expected_effect_plugins, $expected_effect_config) {
   

   $expected_effect_config should have the array type hint.

Derp.

I can't change this in the process yml declaration, because that's for migrating the image styles, while the "id" issue is about the underlying image effect plugins (the "effects" process pipeline in d7_image_styles.yml).
So i'm not sure what the best way is to migrate to D8 plugins that within themselves reference other existing plugins.

The iterator process plugin might be the answer to this. https://www.drupal.org/node/2135345

1. +++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
   @@ -0,0 +1,74 @@
   +      $effects[$key] = [
   +        'name'     => $result['name'],
   +        'weight' => $result['weight'],
   +        'data'   => unserialize($result['data']),
   +      ];
   

   This can just be $effects[$key]['data'] = unserialize($result['data']. No need to copy the name and weight verbatim.

2. +++ b/core/modules/image/src/Tests/Migrate/d7/MigrateImageStylesTest.php
   @@ -0,0 +1,78 @@
   +   * @param string $id
   +   *   The expected image style ID.
   +   * @param string $label
   +   *   The expected image style label.
   +   * @param array $expected_effect_plugins
   +   *   An array of expected plugins attached to the image style entity
   +   * @param array $expected_effect_config
   +   *   An array of expected configuration for each effect in the image style
   

   $expected_size is missing :) It might not even be necessary, since $expected_effect_plugins is being passed in anyway...?

Only thing I can't figure out is why the PHPUnit test doesn't run properly.

Can you elaborate on this?

For #1, I mean this:

foreach ($results as $key => $result) {
  $result['data'] = unserialize($result['data']);
  $effects[$key] = $result;
}

It's OK to include more stuff than just name, weight, and data. Generally I think source plugins should err on the side of returning as much data as is available and allow the migration to decide what, if anything, to do with it.

I'm not sure why the PHPUnit test is failing. Does it produce any warnings or fatals locally?

+++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
@@ -0,0 +1,71 @@
+  public function fields() {
+    $fields = [
+      'isid' => $this->t('The primary identifier for an image style.'),
+      'name' => $this->t('The style machine name.'),
+      'label' => $this->t('The style administrative name.'),
+    ];
+    return $fields;
+  }

We should probably mention effects here, but I think that can be fixed on commit. Many existing source plugins do not return accurate field info.

Fantastic job, @svendecabooter!