Problem/Motivation

While working on #2604484: Migrate Drupal 7 image and file fields I went out of scope and started to work on D6 tests when the issue is just for D7. Since that is 'D7 critical', let's put the D6 work here to not hold that issue up.

In https://www.drupal.org/node/2604484#comment-10694994 I asked why d6_cck_file does a file lookup on the fid, while the D7 migration doesn't. Later I spoke to chx on IRC about this. He said the file lookup wasn't necessary because the drupal-to-drupal migrations keeps the ids, see https://www.drupal.org/node/2604484#comment-10696948.

Proposed resolution

Make the source migration for the file's migration configurable so it isn't hard-coded to d6_file.

Remaining tasks

  • Do it
  • Test it

To test it, we will need a kernel test that wires in a dummy file migration and see if that dummy migration is successful. There isn't really a good way to test this with a unit test.

  • Run dummy file migration. Make sure that the files that are in the dummy migration aren't copied in by any other test.
  • Reference dummy file migration in cckfile migration and validate files from the dummy migration were migrated.

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone’s picture

Status: Active » Needs review
joel_guesclin’s picture

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

chx’s picture

Issue tags: +SprintWeekend2016
quietone’s picture

@joel_guesclin, I just ran a test and it appears that a migration of data from Image Attach needs work. The files are migrated and the node is created but the image file is not associated with the node. So, it seems we need a new issue for migrating from Image Attach. I can't do that now as I'm off to sleep.

What I did: On my test D6 site, I installed Image Attach and used devel to generate 2 image nodes. Then I ran the migration from a fresh D8 install.

JoshOrndorff’s picture

FileSize
73.91 KB

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

quietone’s picture

quietone’s picture

Title: Add tests for file and image field data [d6] » Remove file lookup in process plugin [d6]
Issue tags: +blocker

Yes, this actually does more that add some test, thus the title change. It removes a file lookup in the file process plugin. I talked to chx about it on IRC and he quickly pointed out that it was not needed. I don't recall any more details.

This is blocking #2557557: Move old CCK assertions into MigrateNodeTest

quietone’s picture

@JoshOrndorff, the filefield and imagefield are marked as missing because of the way that the Drupal Upgrade module tries to figure but what migrations are available. I recall it is by module, which doesn't work so well for fields, such and imagefield and filefield.

The migration of imagefields and filefields is working in D6. It is not missing. But, if you have problems, please file an issue. Or, you can ask in #drupal-migrate, which is also a good place to learn more about migrate.

quietone’s picture

Issue tags: +migrate-d6-d8
benjy’s picture

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?

quietone’s picture

Issue summary: View changes

Added some info to the IS.
I don't remember the details, but this may be related to getting errors on image files, because the height and width are empty. I once tracked it to ImageItem:preSave,

benjy’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alvinalexander’s picture

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?

benjy’s picture

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

Status: Needs review » Needs work

The last submitted patch, 8: 2640842-8.patch, failed testing.

mikeryan’s picture

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...

thlor’s picture

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.

jonreid’s picture

@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

thlor’s picture

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.

jonreid’s picture

@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

thlor’s picture

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!

jonreid’s picture

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

mikeryan’s picture

Title: Remove file lookup in process plugin [d6] » Make related file migration ID configurable in d6_cck_file

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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

Issue tags: +Novice

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)
    );
  }
juancasantito’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
1004 bytes
mikeryan’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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

heddn’s picture

Issue summary: View changes

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

heddn’s picture

heddn’s picture

The last submitted patch, 34: make_related_file-2640842-34-tests_only.patch, failed testing.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
  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().

heddn’s picture

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.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

I think we're ready here, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: make_related_file-2640842-37.patch, failed testing.

quietone’s picture

Retesting. Error in unrelated file and that test passed locally.

quietone’s picture

Status: Needs work » Reviewed & tested by the community

Tests passing, back to RTBC.

  • catch committed bd4f179 on 8.3.x
    Issue #2640842 by heddn, quietone, juancasantito, JoshOrndorff: Make...

  • catch committed ca64142 on 8.2.x
    Issue #2640842 by heddn, quietone, juancasantito, JoshOrndorff: Make...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

mikeryan’s picture

Issue tags: +Needs change record

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

mikeryan’s picture

Status: Fixed » Needs review
Issue tags: -Needs change record

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

heddn’s picture

Status: Needs review » Fixed

Published.

Status: Fixed » Closed (fixed)

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