Problem/Motivation

Per #2533886: [meta] Move module-specific migration support into the particular modules supported, move support for action migration into the action module.

According to usage statistics on the ImageCache project page 60,000 to 70,000 sites are still using the ImageCache module on D6 sites. This in connection with its inclusion into core for D7 heavily suggests that we should include a core migration package to handle these use cases.

Proposed resolution

Add migration to core

Remaining tasks

none

User interface changes

none

API changes

none

Data model changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lostkangaroo’s picture

Assigned: lostkangaroo » Unassigned
Issue tags: +Needs tests
FileSize
6.71 KB

This could stand to take a good review as some of the base mechanics are still fuzzy to me, but this patch works. I would like to see the id mapping not be located in its own process plugin but included in the template if possible for easier extensibility.

lostkangaroo’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: d6_imagecache_d8-2510160-1.patch, failed testing.

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityImageStyle.php
    +  public function import (Row $row, array $old_destination_id_values = array()) {
    +    $effects = array();
    +
    +    if ($row->getDestinationProperty('effects')) {
    +      $effects = $row->getDestinationProperty('effects');
    +      $row->setDestinationProperty('effects', NULL);
    +    }
    +
    +    $entity_ids = parent::import($row, $old_destination_id_values);
    +    $style = $this->storage->load(reset($entity_ids));
    +
    +    foreach ($effects as $effect) {
    +      $style->addImageEffect($effect);
    +    }
    +
    +    $style->save();
    +
    +    return $entity_ids;
    +  }
        

    This is oddly structured because it's going out of its way to call parent::import(). As far as I know, it won't cause any problems if the style is built and saved in this method, without bothering to call parent::import(). As long as it returns the saved entity ID, things should continue to work. IMO this should be refactored, but if it needs to keep this structure then there should at least be comments explaining why.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/ImageCacheActions.php
    +      switch($action['action']) {
    +        case 'imagecache_resize':
    +          $id = 'image_resize';
    +          break;
    +        case 'imagecache_scale':
    +          $id = 'image_scale';
    +          break;
    +        case 'imagecache_scale_and_crop':
    +          $id = 'image_scale_and_crop';
    +          break;
    +        case 'imagecache_crop':
    +          $id = 'image_crop';
    +          break;
    +        case 'imagecache_desaturate':
    +          $id = 'image_desaturate';
    +          break;
    +        case 'imagecache_rotate':
    +          $id = 'image_rotate';
    +          break;
    +        case 'imagecache_deprecated_scale':
    +        case 'imagecache_sharpen':
    +        default:
    +          $id = null;
    +          break;
    +      }
        

    Definitely not a big deal, but this could be made shorter by using preg_replace(). There's also no need to set $id to NULL in the default case, because empty() will do an implicit isset() check.

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ImageCachePreset.php
    +  public function query() {
    +    $query = $this->select('imagecache_preset', 'icp')
    +      ->fields('icp', array(
    +        'presetid',
    +        'presetname',
    +      )
    +    );
    +    return $query;
    +  }
        
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ImageCachePreset.php
    +    $results = $this->database
    +      ->select('imagecache_action', 'ica', array('fetch' => \PDO::FETCH_ASSOC))
    +      ->fields('ica', array(
    +        'actionid',
    +        'presetid',
    +        'weight',
    +        'action',
    +        'data',))
    +      ->condition('presetid', $row->getSourceProperty('presetid'))
    +      ->execute();
    +
        

    Use $this->select(), not $this->database->select(). $this->select() will set the proper fetch option for you. There's also no need to select all the fields manually -- you can just use fields($table). The reason other source plugins do it the long way is because it's a relic of the Fake DB driver, which is now gone.

  4. Obviously this needs tests for each plugin, plus an integration test.

Other than that, this looks pretty good to me.

lostkangaroo’s picture

Status: Needs work » Needs review
FileSize
6.34 KB
4.69 KB

Did some renovation work to how this migration moves the data taking the feedback into accord.

1) Removed the parent::import method call. Also added a try catch to catch any PluginNotFoundExceptions that will occur if the ImageStyle effect does not have a corresponding plugin in D8 and throw a Migration Exception. This is the case in two options allowed in D6: imagecache_deprecated_scale, imagecache_sharpen.

2) Yeah hated the switch. Its primary need was to filter out the two effect cases stated above and set the d8 effect names but with the rewritten method it is no longer needed.

3) Thanks for the pointers on the query useage most of that was modified copy and paste from other relic sources.

Still needs tests but will hopefully get those in soon. Just looking to see if this new code passes the smell test better before continuing.

phenaproxima’s picture

Status: Needs review » Needs work

Looks a lot better. I like the use of exception handling to catch invalid plugins.

  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityImageStyle.php
    @@ -22,24 +24,34 @@ class EntityImageStyle extends EntityConfigBase {
     
       /**
        * {@inheritdoc}
    +   *
    +   * @throws MigrateException
        */
    

    MigrateException should be fully-qualified, and please add an explanation about the circumstances under which the exception is thrown.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/ImageCacheActions.php
    +  public function transform ($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    

    Total nitpick: no space between the method name and the parameters.

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/ImageCacheActions.php
    +        'id'     => str_replace('imagecache', 'image', $action['action']),
    

    I don't recommend str_replace() here. Use preg_replace(), because you only want to change it if the action name starts with imagecache_. str_replace() will replace it anywhere it appears.

lostkangaroo’s picture

Round 3, now to add tests.

lostkangaroo’s picture

Status: Needs work » Needs review
lostkangaroo’s picture

First pass at getting tests in place.

Status: Needs review » Needs work

The last submitted patch, 10: d6_imagecache_d8-2510160-10.patch, failed testing.

phenaproxima’s picture

The dump files listed in setUp() are missing the .php extension.

lostkangaroo’s picture

Status: Needs work » Needs review
FileSize
15.64 KB
744 bytes

Always something simple in the first draft

Status: Needs review » Needs work

The last submitted patch, 13: d6_imagecache_d8-2510160-13.patch, failed testing.

lostkangaroo’s picture

Component: migration system » image.module
Issue summary: View changes
lostkangaroo’s picture

Issue summary: View changes
lostkangaroo’s picture

Issue summary: View changes
neclimdul’s picture

reroll. leaving nw.

lostkangaroo’s picture

Status: Needs work » Needs review
FileSize
18.49 KB
13.19 KB

Still based on the current migrate_drupal and not ready to be committed. Remaining tasks include testing exceptions raised during migrations caused by missing effect plugins, and crop effects used in D6 that do not use keyword anchors.

lostkangaroo’s picture

@phenaproxima mentioned in IRC that updated variable and system tables might be beneficial.

lostkangaroo’s picture

Issue summary: View changes
lostkangaroo’s picture

FileSize
2.24 KB
20.73 KB
lostkangaroo’s picture

Status: Needs review » Postponed
lostkangaroo’s picture

lostkangaroo’s picture

Status: Postponed » Needs work

blocker is gone so no longer postponed

neclimdul’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
21.59 KB

updated patch.

Fixes changes from #2499793: Several migrate_drupal migrations fatal error on count(). Since we don't run the migration in the setup there are failures. Also, MigrateTestBase doesn't clean up its variables in tear which leads to really useless errors if you aren't running the migration.

Gets the failure test working.

lostkangaroo’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 26: d6_imagecache_d8-2510160-26.patch, failed testing.

lostkangaroo’s picture

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateImageCacheTest.php
@@ -0,0 +1,125 @@
+    /** @var \Drupal\migrate\Entity\Migration $migration */
+//    $migration = entity_load('migration', 'd6_imagecache_presets');

Might want to remove this in the next pass.

neclimdul’s picture

FileSize
20.92 KB

still needs work i think but here's a reroll.
Addressed #29

neclimdul’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
18.68 KB
8.53 KB
lostkangaroo’s picture

Status: Needs review » Reviewed & tested by the community

This looks good and since no remaining tasks are present lets get this in.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

I'm sorry to do this after hitting RTBC, but I found some things (mostly minor quibbles)...

+++ b/core/modules/image/src/Plugin/migrate/destination/EntityImageStyle.php
+class EntityImageStyle extends EntityConfigBase {
+
+  /**
+   * {@inheritdoc}
+   *
+   * @throws \Drupal\migrate\MigrateException
+   */

Please remove the @throws line. IIRC, the API documentation generator doesn't like it if we mix @inheritdoc with other things.

+++ b/core/modules/image/src/Plugin/migrate/destination/EntityImageStyle.php
+    foreach ($effects as $effect) {
+      try {
+        $style->addImageEffect($effect);
+      }
+      catch (PluginNotFoundException $e) {
+        throw new MigrateException($e->getMessage());
+      }

If possible, please pass $e as the previous exception wrapped by the MigrateException, so that we don't lose the context in which the exception was originally thrown.

+++ b/core/modules/image/src/Plugin/migrate/process/d6/ImageCacheActions.php
+  /**
+   * {@inheritdoc}
+   *
+   * @throws \Drupal\migrate\MigrateSkipRowException.
+   */

No @throws here.

+++ b/core/modules/image/src/Plugin/migrate/process/d6/ImageCacheActions.php
+    foreach($row->getSourceProperty('actions') as $action) {
+      ...
+    }
<code>
<p>Nitpick, but you could have the migration use the iterator plugin to loop over and process each action, which means $value would be a single action and make this a little easier to read. It's fine as-is though, so that's just a suggestion.</p>
<code>
+++ b/core/modules/image/src/Plugin/migrate/source/d6/ImageCachePreset.php
+/**
+ * Drupal 6 imagecache presets source from database.
+ *
+ * @MigrateSource(
+ *   id = "d6_imagecache_presets"
+ * )
+ */
+
+class ImageCachePreset extends DrupalSqlBase {

Thar be an extra line between the doc block and the class.

+++ b/core/modules/image/src/Plugin/migrate/source/d6/ImageCachePreset.php
+  public function query() {
+    $query = $this->select('imagecache_preset', 'icp')
+      ->fields('icp', [
+        'presetid',
+        'presetname',
+        ]
+    );
+    return $query;
+  }

It should be OK to just do ->fields('icp') here.

+++ b/core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php
+/**
+ * Migrate ImageCache presets to Image styles
+ *
+ * @group image
+ */

Please put this in the migrate_drupal_6 test group so that MigrateDrupal6Test will pick it up. The d6_imagecache_presets migration should also be added to its giant laundry list of migration IDs.

+++ b/core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php
+    $this->prepareMigrations(array(
+      'd6_imagecache_presets' => [],
+    ));

This pattern is the cause of our MigrateDrupal6Test woes. It's preferable to just execute the dependency directly: $this->executeMigration('d6_imagecache_presets')

+++ b/core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php
+    //check crop effect

Super-annoying code style nit: there should be a space between the comment text and the //. Also please use capitalization and punctuation, just for consistency with the rest of core.

+++ b/core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php
+   * Test that missing action's causes failures.

Moar grammar policing. Should be "Test that missing actions cause failures."

+++ b/core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php
+  protected function assertImageEffect($collection, $id, $config) {
+    /** @var \Drupal\image\ConfigurableImageEffectBase $effect */
+    foreach ($collection as $key => $effect) {
+      $effect_config = $effect->getConfiguration();
+
+      if ($effect_config['id'] == $id && $effect_config['data'] == $config) {
+        return $this->pass('Effect ' . $id . ' imported correctly');
+      }
+    }
+    return $this->fail('Effect ' . $id . ' did not import correctly');

This seems strange to me. What would be wrong with simply $this->assertIdentical($effect_config['id'], $id) and $this->assertEqual($effect_config['data'], $config)?

+++ b/core/modules/migrate_drupal/config/schema/migrate_drupal.source.schema.yml
+migrate.source.d6_imagecache_presets:
+  type: migrate_source_sql
+  label: 'Drupal 6 ImageCache Presets'
+

This needs to go in core/modules/image/config/schema/image.source.schema.yml.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
18.41 KB
5.81 KB

Think I got everything. Things that didn't change.
1) Didn't change group. I purposely excluded this from MigrateDrupal6Test
2) Didn't move the Migration run to the setup. The tests methods modify the source tables to assert different conditions so have to run the migration after that happens. This is the reason for #1.
3) assertImageEffect does take a bit to wrap your head around. The trick is the interaction of the return inside the loop. The loop is iterating looking for a matching effect. Non-matching effects are actually ok, the failure is that when it is unable to find the matching effect. Did comment to make this a bit clearer though.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Good enough for me.

webchick’s picture

Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed d3f9129 on 8.0.x
    Issue #2510160 by lostkangaroo, neclimdul, phenaproxima: d6 Imagecache...

Status: Fixed » Closed (fixed)

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