Problem/Motivation

Managed Drupal 7 files need to be migrated to Drupal 8 file entities.

Proposed Resolution

D7's unified file management API (and the fact that files are already entities!) makes this a lot easier than migrating files from Drupal 6 :) We need a simple migration (d7_file) that simply transfers all file_managed rows to Drupal 8 file entities, and copies the actual files to the corresponding Drupal 8 path.

Remaining Tasks

  1. Write the migration
  2. Write tests
  3. Review
  4. Commit
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima’s picture

Status: Active » Needs review
FileSize
6.83 KB

Initial patch.

mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal/config/optional/migrate.migration.d7_file.yml
    @@ -0,0 +1,27 @@
    +# Every migration that saves into {file_managed} must have the d7_file
    

    Probably some D6 holdover from the user picture handling - better would be to say something like "Every migration that references files by fid must have...".

  2. +++ b/core/modules/migrate_drupal/config/optional/migrate.migration.d7_file.yml
    @@ -0,0 +1,27 @@
    +  # filesize is dynamically computed when files are saved, so there is no point
    +  # in migrating this value.
    +  # filesize: filesize
    

    No biggie, but I'd just as soon leave the straight mapping (thinking ahead to contrib extensions, people wondering why that one field was left unmapped).

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/File.php
    @@ -0,0 +1,52 @@
    +      'timestamp' => $this->t('The time that the file was added.'),
    

    D8 has replaced 'timestamp' with 'created and 'changed' (presumably the D6 version needs to also be fixed here).

  4. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
    @@ -0,0 +1,65 @@
    +    touch('public://cube.jpeg');
    +    file_put_contents('public://cube.jpeg', str_repeat('*', 3620));
    

    So, we're putting a test file into the D8 files directory, where the migration is supposed to copy it from a D7 site?

phenaproxima’s picture

#1) Fixed.

#2) I left it in there, with the comment explaining why it's not migrated, so that people can find out if they're curious.

#3) I didn't fix, because I thought that fields() was supposed to be the list of properties returned by the source. changed and created are mapped in by the process pipeline, but I don't see why the source should need to know about them.

#4) Good point. I have rejiggered the d7_file source plugin to provide a computed property called filepath, which is the absolute path represented by the original URI. It's not thoroughly tested (ideally I'd also test for temporary and private URIs) but it's a start.

mikeryan’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Changing status for testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2503861-3.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
8.88 KB

Trying again, basing the test's file operations in its public files directory rather than /tmp.

benjy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal/config/optional/migrate.migration.d7_file.yml
    @@ -0,0 +1,27 @@
    +  created: timestamp
    +  changed: timestamp
    

    O, D7 didn't keep track of updated/created.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/File.php
    @@ -0,0 +1,75 @@
    +    $search = array('public:/', 'private:/', 'temporary:/');
    +    $replace = array(
    +      $this->variableGet('file_public_path', 'sites/default/files'),
    +      $this->variableGet('file_private_path', NULL),
    +      $this->variableGet('file_temporary_path', '/tmp'),
    +    );
    

    If we don't need the context of the row then we should do this in initializeIterator(), that way it only runs once per migration.

  3. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
    @@ -0,0 +1,85 @@
    + * @dumpFile d7/FileManaged.php
    + * @dumpFile d7/Variable.php
    

    I know you have plans for this but we should remove these until we have a use for them.

  4. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
    @@ -0,0 +1,85 @@
    +    // The public files directory active during the test will serve as the
    +    // root of the fictional Drupal 7 site we're migrating.
    

    Should this comment be a few lines higher?

  5. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
    @@ -0,0 +1,85 @@
    +      'plugin' => 'entity:file',
    

    This is already set in the migration itself?

  6. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
    @@ -0,0 +1,85 @@
    +      // This is set in the migration's YAML file, but we need to repeat it
    +      // here because all the destination configuration must be set at once.
    

    I'm not sure about this, surely you can set individual config keys?

  7. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
    @@ -0,0 +1,85 @@
    +      'source_path_property' => 'filepath',
    

    As per above.

  8. +++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
    @@ -0,0 +1,85 @@
    +  protected function assertEntity($id, $name, $uri, $mime, $size, $created, $changed, $uid) {
    

    Missing function comment.

  9. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/d7/FileTest.php
    @@ -0,0 +1,68 @@
    +namespace Drupal\Tests\migrate_drupal\Unit\source\d7;
    +
    +use Drupal\Core\Database\Connection;
    +use Drupal\Core\Extension\ModuleHandlerInterface;
    +use Drupal\migrate_drupal\Plugin\migrate\source\d7\File;
    +
    +class TestFile extends File {
    +  public function setDatabase(Connection $database) {
    +    $this->database = $database;
    +  }
    +  public function setModuleHandler(ModuleHandlerInterface $module_handler) {
    +    $this->moduleHandler = $module_handler;
    

    Is this used?

phenaproxima’s picture

Mostly fixed. Didn't bother with #5 through #8, as per discussion on IRC. Regarding #9 -- I don't know if it's used, but I'm following the same pattern as every D6 unit test, which has the exact same structure. When KTBNG lands, we should be able to convert these to kernel tests (which they properly are) and get rid of these stub plugins.

phenaproxima’s picture

Status: Needs work » Needs review

Status for testbot.

benjy’s picture

Regarding #9, if it's not used we should just remove it. I don't know what you mean by "exact same structure". It's a dummy class used to make testing easier, we either need it or we don't.

phenaproxima’s picture

@benjy -- We do need it, because it's the class that actually gets tested. All the other unit tests follow the same pattern, but truth be told I'm not sure we need to do that anymore, now that the tests use SQLite as a backend. I should open a follow-up issue to remove all these dummy classes, if possible; they're tedious to write, and they're all freaking identical anyway.

benjy’s picture

FileSize
3.04 KB
11.7 KB

We do need it, because it's the class that actually gets tested

I was struggling to see how that was the case but I looked closer and found the madness :)

$plugin_class = preg_replace('/^Drupal\\\\(\w+)\\\\Plugin\\\\migrate(\\\\source(\\\\.+)?\\\\)([^\\\\]+)$/', 'Drupal\\Tests\\\$1\\Unit$2Test$4', static::PLUGIN_CLASS);

The attached patch keeps backwards compatibility for the [Class]Test approach but it handles module handler and database so we can start cleaning those classes up!

phenaproxima’s picture

Brilliant!! If we can do that reflection trick for this one class, we might as well go all the way and remove all of these pointless Test* classes. But let's do that in a follow-up.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

OK, the rest looks good. Lets RTBC #9 and then handle the test class clean-up here #2505521: Clean-up un-need test classes in migrate_drupal

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2503861-13.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC. It's #9 we want to commit, and it still passes.

benjy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
9.49 KB
843 bytes

#2505521: Clean-up un-need test classes in migrate_drupal landed so we can remove that class now.

Status: Needs review » Needs work

The last submitted patch, 18: 2503861-18.patch, failed testing.

phenaproxima queued 18: 2503861-18.patch for re-testing.

The last submitted patch, 18: 2503861-18.patch, failed testing.

The last submitted patch, 18: 2503861-18.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Postponed

Postponing on #2510072: UTF-8 support in MySQL driver breaks migrate dump files, since tests will not work until that one is committed.

phenaproxima’s picture

Status: Postponed » Needs review

phenaproxima queued 18: 2503861-18.patch for re-testing.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC for #18, since @benjy's change was just cleanup and didn't change any of the functionality.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2503861-18.patch, failed testing.

phenaproxima queued 18: 2503861-18.patch for re-testing.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Testbot being weird.

catch’s picture

Status: Reviewed & tested by the community » Needs work

It doesn't look from the patch like it takes into account the possibility of duplicate files in the source table - see #1260938: d6 to d7 update fails on file duplicates '#7061 Integrity constraint violation'.

The test data added there could be re-used here.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Duplicate paths are only an issue in D6; this migration is for D7. (@catch OKed re-RTBCing on IRC.)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2503861-18.patch, failed testing.

phenaproxima queued 18: 2503861-18.patch for re-testing.

The last submitted patch, 18: 2503861-18.patch, failed testing.

phenaproxima queued 18: 2503861-18.patch for re-testing.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC -- @xjm informed me that HEAD broke testbot today due to a bungled commit. But that's all fixed now, so I think we're good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
--- /dev/null
+++ b/core/modules/migrate_drupal/config/optional/migrate.migration.d7_file.yml

Needs to be a template.. see the CR for this.

alexpott’s picture

+++ b/core/modules/migrate_drupal/src/Tests/d7/MigrateFileTest.php
@@ -0,0 +1,102 @@
+ * @group migrate_drupal 7.x

I think the @group has been standardised.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
9.34 KB
2.43 KB

Fixed!

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

phenaproxima’s picture

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
9.09 KB
0 bytes
chx’s picture

Status: Needs review » Reviewed & tested by the community

While normally I leave RTBCing an issue where benjy is involved to him, this was already RTBC'd by him and only went through non-significant changes so I think I am OK with this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
@@ -0,0 +1,99 @@
+    $path = str_replace(['public:/', 'private:/', 'temporary:/'], [$this->publicPath, $this->privatePath, $this->temporaryPath], $row->getSourceProperty('uri'));

We're missing test coverage of the private and temporary replaces. The private one is important cause security. Also when we test we only test migrating one file - I would have thought we should always test more than one of things like files which are likely to be migrated in their hundreds.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
10.51 KB
1.89 KB

Added tests of the URI transformation.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks good - thanks. I don;t foresee any problems with concurrent testing from either of the tests being added here. Nice.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

So.. I want to commit this but I found this:

+  protected function setProperty($property, $value) {
+    $reflector = (new \ReflectionClass($this->source))->getProperty($property);
+    $reflector->setAccessible(TRUE);
+    $reflector->setValue($this->source, $value);
+  }

1) There's no doxygen here, but should be.
2) Why are we doing these Reflection shenanigans?
3) I thought this might've been an @inheritdoc situation, but this method doesn't exist in MigrateSqlSourceTestCase. Should it?

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
10.83 KB
3.25 KB

Removed reflection stuff in favor of subclassing.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks!

Committed and pushed to 8.0.x.

  • webchick committed de94d5e on 8.0.x
    Issue #2503861 by phenaproxima, benjy, alexpott, mikeryan: Migrate...

Status: Fixed » Closed (fixed)

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