Make related file migration ID configurable in d6_cck_file

Does this handle images attached to other content types as per this? Migration of Image Attach module

I'd like to help with this however I can. I'm a fairly experienced programmer with little php experience. I've read the entire issue: #2604484: Migrate Drupal 7 image and file fields

I'm also in the early stages of migrating a D6 site that has lots of image fields to D8. I see the patch has all tests passing, but feel I must be missing something because when I use the /upgrade ui in a new D8 installation I see that filefield and imagefield are listed as "missing" in the destination column.

I've been managing this my site with drupal (as a site admin, but not developer) for a while, so I'm not a noob, but I feel I must be missing something because the previous issue made it sound like this functionality is at least complete enough to be broken as opposed to completely missing. I got the impression that writing tests would show us where, if anywhere, we needed to modify the work that quietone had done for the D7 -> D8 upgrade to fit with D6 -> D8.

If someone can point out what I'm missing, I'd certainly be able to test, and am also really motivated to learn how the migrate system with its processes and iterators work, so I could potentially write some migrations as well.

I'm not familiar with image attach unfortunately.

Thanks in advance for any tips, and thanks to queitone for all the work you've put into this already.
-Josh

Why are we actually removing this, reading the comment it sounds like it was needed at some point but I can't see a reference to where those issues may have been fixed?

Looks like it was here; #2549013: Remove load plugins

Adam has a comment there with some reasoning: https://www.drupal.org/node/2549013#comment-10265575

I'm not sure if this is the right place to report this, but I created a Photo content type on my Drupal 6 websites that is not being migrated to Drupal 8. This page (https://www.drupal.org/node/2167633) that states, "Images attached to Drupal 6 Image nodes, and files attached with File fields do not get migrated," referred me to here.

I defined that Drupal 6 content type with Label=Photo, Name=field_photo, and Type=File. I also used ImageCache to make preview images, which were shown on the Photo nodes. (I think I got this "recipe" in the "Using Drupal" book.)

On the Drupal 8 website I see that I have a similar Photo content type, the URL alias for each node (over 1,700 of them), and the node's title and body content. The large, original image files are also under the 'sites/default/files' folder, but the 'imagecache/preview' files are not on the Drupal 8 site.

I did find the URI to the original photo files in the `uri` field of the `file_managed` table, but I can't find any reference to the ImageCache'd files. Those files are what were being shown on my main Photo node, like this example page: http://alvinalexander.com/photos/tablet-shipments-least-100m-less-predicted

The same URI on my Drupal 8 test site shows the node title and content, but has no reference to an image at all, and has no <img> tags in the HTML.

Given all of that, my question is, is there any way to get these Drupal 6 photo nodes to work under Drupal 8?

I'm still not sure whether we can remove this try/catch or now, I think that keeping it could be safer?

The straight upgrade path supported in core maintains IDs, true, but custom Drupal-to-Drupal migrations don't need to (and many projects cannot, as when importing into a site with existing content). To make it simpler to leverage the core migrations in contrib and custom modules, it's actually preferable if the core migrations not take for granted that IDs are being preserved in references and instead do the lookup. In other words - I'm lazy, in #2729613: [meta] Implement custom Drupal migrations for Drupal 8 I don't want to have to rewrite a lot of the core migrations to make references work...

File lookup on the fid should stay. I just went through the scenario described in #20: custom Drupal-to-Drupal file migration (since I wanted to override some of d6_file.yml). All I had to do is extend the CckField class and override its 'create' method and let $migration_plugin_configuration['migration'] = 'my_custom_file_migration';
Fid mapping now works.

@mikeryan: I think an issue I created is related to this, if not a duplicate.

Files not attached to nodes using drush migrate-import:
https://www.drupal.org/node/2767237

@thlor: Did you extend the CCkfield class in a custom migration module? Could you provide a little more detail or a link to a similar example please?

Thanks

https://gist.github.com/thlor/1585192d93412da4d5a5155ad62a4420
@jonreid Hope this clears up what I meant. If not, please let me know and I will try to write better comments.

@thlor: That's a big help, thank you.

I'm replicating a simplified example migration using your gist. Is there anything I'm missing to register that custom plugin?

Getting:

exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "custom_file" plugin does not exist.

when I drush migrate-import

Hard to say - make sure that the plugin is in the correct directory ({custom_module}/src/Plugin/migrate/process/CustomFile.php) and double check namespacing. I made a change there to make it obvious that you have to replace the module name with your module's machine name in the namespace too. Check the gist again!

Thanks @thlor! The plugin not found error is resolved. I had the namespace mislabelled slightly.

People doing custom Drupal migrations regularly run into this (e.g., #2767237: Files not attached to nodes using drush migrate-import, and me) if they have an ID other than d6_file for their file migration. The right answer (should be pretty straight-forward) would be to add a "migration" configuration key to d6_cck_file, so the referenced migration is easily customizable.

The tests on this are probably harder than actually coding this. All that needs to be done is in the create() constructor for CckFile (d6_cck_file), we need to allow the over-riding the default d6_file by passing in a custom name. Tagging novice for the creation of the patch that does this. Tests are a bonus and are usually less novice.

Maybe something like:

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
    // Configure the migration process plugin to look up migrated IDs from
    // the d6_file migration.
    $migration_plugin_configuration = ['source' => ['fid']];
    $migration_plugin_configuration += ['migration' => 'd6_file'];

    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $migration,
      $container->get('plugin.manager.migrate.process')->createInstance('migration', $migration_plugin_configuration, $migration)
    );
  }

All right, that looks good functionally - now we need a test.

Updated IS with some direction on what should be done for the tests.

The interdiff is the tests only pach.

Let's upload the right test. #33 will likely fail.

1. +++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
   @@ -50,11 +50,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    $migration_plugin_configuration = ['source' => ['fid']] + $configuration;
   +    $migration_plugin_configuration += ['migration' => 'd6_file'];
   

   This is a nitpick for sure, but why is this split across two lines? Can we collapse it?

2. +++ b/core/modules/file/src/Plugin/migrate/process/d6/CckFile.php
   --- /dev/null
   +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateCckFileTest.php
   

   Let's move this into the Drupal\Tests\file\Kernel\Migrate\process\d6 namespace and rename it to CckFileTest.

3. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateCckFileTest.php
   @@ -0,0 +1,86 @@
   + * @group migrate_drupal_6
   

   Shouldn't this be in the file group?

4. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateCckFileTest.php
   @@ -0,0 +1,86 @@
   +    $cache = $this->prophesize(CacheBackendInterface::class)->reveal();
   +    $moduleHandler = $this->prophesize(ModuleHandlerInterface::class)->reveal();
   +    $migrationPluginManager = new TestableMigratePluginManager('process', new \ArrayObject(), $cache, $moduleHandler);
   +    $this->container->set('plugin.manager.migrate.process', $migrationPluginManager);
   

   Because this is a kernel test, I'd rather we didn't mock these services. We'll more likely to catch real regressions if we use what the real services.

5. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateCckFileTest.php
   @@ -0,0 +1,86 @@
   +    $this->assertEquals('custom_file', $testableMigration->getMigrationPlugin()->getConfiguration()['migration']);
   

   We don't need to bother with TestableCckFile or any of the rigging for it -- PHPUnit provides a method for pulling inaccessible properties out of objects :) See PHPUnit_Framework_Assert::readAttribute().

This needs to stay a kernel test so we have access to the container. But otherwise, using phpunit's readAttribute is a great tip. All points in #36 are addressed. I've added an interdiff, but it might be more confusing since the test moved and shrunk. The full patch is fairly short, so I suggest to just review it.

I think we're ready here, thanks!

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

We should have added a change record so people know they can take advantage of this... Will do after lunch.

Draft change record added - anyone care to review and (assuming it looks good) publish?

Published.