Problem/Motivation

D7 had a preserve_files option to trigger this behavior, we don’t have that in D8. This results in really slow migration when you're talking about multiple gigabytes of files.

Proposed resolution

Add a 'reuse' file configuration option. If that flag is set on the file migration, it will attempt to re-use files that are already in the destination with the same name.

Remaining tasks

Code a patch
Review

User interface changes

API changes

Additional process plugin option for file migrations to configure a 'preserve_files' flag.

Data model changes

CommentFileSizeAuthor
#35 file_migrations-2706405-35.patch4.08 KBheddn
#35 interdiff_27-35.txt2.07 KBheddn
#33 interdiff_31-33.txt1.17 KBheddn
#33 file_migrations-2706405-33.patch4.2 KBheddn
#31 file_migrations-2706405-31.patch4.2 KBedysmp
#31 interdiff-27-31.txt2.49 KBedysmp
#27 file_migrations-2706405-27_tests-only.patch2.56 KBheddn
#27 interdiff_25-27.txt1.04 KBheddn
#27 file_migrations-2706405-27.patch4.63 KBheddn
#25 interdiff_23-25.txt486 bytesheddn
#25 file_migrations-2706405-25.patch4.53 KBheddn
#23 drupal-file_migrations-2706405-23.patch4.58 KBheddn
#20 drupal-file_migrations-2706405-20.patch4.64 KBheddn
#18 drupal-file_migrations-2706405-18.patch3.78 KBheddn
#18 interdiff_15-18.txt519 bytesheddn
#15 interdiff_12-15.txt1.14 KBheddn
#15 drupal-file_migrations-2706405-15.patch3.76 KBheddn
#14 interdiff_12-14.txt505 bytesheddn
#14 drupal-file_migrations-2706405-14.patch3.64 KBheddn
#12 drupal-file_migrations-2706405-12.patch3.63 KBldavidsp
#3 file_migrations_preserve_files-2706405-3.patch2.62 KBedysmp
#2 file_migrations_preserve_files-2706405-2.patch783 bytesedysmp
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

edysmp’s picture

Status: Active » Needs review
FileSize
783 bytes

Patch for configuration option.

edysmp’s picture

benjy’s picture

Anonymous’s picture

Issue tags: +neworleans2016
benjy’s picture

mikeryan’s picture

Status: Needs review » Postponed

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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

Status: Postponed » Needs work

Ready for more work now.

mikeryan’s picture

I think "preserve_files" is going to be confusing to people who've used the D7 migrate module, because what it's doing here is not what is was doing there. In D7, "preserve_files" meant to not delete files on rollback (so that rolling back nodes with file references would not remove files when file_usage would otherwise be empty for a given file). The D7 means of indicating the desired behavior here (if the destination file already exists, use the existing file instead of recopying it) was to extend the core $replace flags of FILE_EXISTS_RENAME and FILE_EXISTS_REPLACE with an additional MigrateFile::FILE_EXISTS_REUSE.

heddn’s picture

I have some logic I've included in #2795879: FileBlob process plugin that I wonder if we should pull into core and solve this more elegantly. We already have FILE_EXISTS_ERROR, why not just use it and not copy if the file already exists.

ldavidsp’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
3.63 KB

No interdiff since this takes a different approach now that #2695297: Refactor EntityFile and use process plugins instead is in core.

Status: Needs review » Needs work

The last submitted patch, 12: drupal-file_migrations-2706405-12.patch, failed testing.

heddn’s picture

Question: why is the class under test named FileCopy and the test named CopyFile?

heddn’s picture

The last submitted patch, 14: drupal-file_migrations-2706405-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: drupal-file_migrations-2706405-15.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
519 bytes
3.78 KB

Status: Needs review » Needs work

The last submitted patch, 18: drupal-file_migrations-2706405-18.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
4.64 KB

Trying this.

Status: Needs review » Needs work

The last submitted patch, 20: drupal-file_migrations-2706405-20.patch, failed testing.

benjy’s picture

Title: File migrations *always* copy in files, even if the files where off-line copied to destination » File migrations *always* copy in files, even if the files were off-line copied to destination
heddn’s picture

Status: Needs work » Needs review
FileSize
4.58 KB

Status: Needs review » Needs work

The last submitted patch, 23: drupal-file_migrations-2706405-23.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
486 bytes

None of the other tests seem to clean-up after themselves. So for now I'm going to punt on that and leave it to a follow-up if we decide that cleaning up files in the teardown is a necessary step.

Status: Needs review » Needs work

The last submitted patch, 25: file_migrations-2706405-25.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: file_migrations-2706405-27_tests-only.patch, failed testing.

heddn’s picture

Issue summary: View changes
Status: Needs work » Needs review

This is ready for review.

phenaproxima’s picture

  1. +++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
    @@ -33,7 +35,12 @@ class CopyFileTest extends FileTestBase {
    +    $this->container->get('stream_wrapper_manager')->registerWrapper('public', PublicStream::class, StreamWrapperInterface::LOCAL_NORMAL);
    

    I believe the public stream wrapper is already registered by the system module in a kernel test -- does it still pass if we remove this line?

  2. +++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
    @@ -33,7 +35,12 @@ class CopyFileTest extends FileTestBase {
    +  protected function tearDown() {
    +    parent::tearDown();
       }
    

    Looks like this isn't adding anything useful, can we remove it?

  3. +++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
    @@ -71,6 +78,29 @@ public function testSuccessfulCopies() {
    +    self::assertEquals((new \SplFileInfo($destination_path))->getMTime(), $timestamp);
    

    Why use self::? Can we switch to $this->assertGreaterThan() and $this->assertLessThan() when asserting against the timestamp?

    Let's also add a couple of calls like $this->assertInternalType('int', $modified_time), since, as far as I can infer from the PHP documentation, SplFileInfo::getMTime() might return FALSE on failure.

Additionally, it'd be cool if we also added a test of an unsuccessful reuse.

edysmp’s picture

Status: Needs review » Needs work

The last submitted patch, 31: file_migrations-2706405-31.patch, failed testing.

heddn’s picture

Let's flip expected with actual. That should work better.

Status: Needs review » Needs work

The last submitted patch, 33: file_migrations-2706405-33.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
4.08 KB

Passes on local.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -142,13 +148,17 @@ protected function writeFile($source, $destination, $replace = FILE_EXISTS_REPLA
    +   *   FILE_EXISTS_REPLACE (default) FILE_EXISTS_RENAME, or FILE_EXISTS_ERROR
    

    Nit: comma after (default) (can be fixed on commit).

  2. +++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
    @@ -71,6 +73,32 @@ public function testSuccessfulCopies() {
    +    sleep(1);
    

    I kinda got an icky feeling at adding a second to test time - but, I don't see an alternative and besides, if I'm counting correctly, the full test suite is already losing 22 seconds to sleep() calls...

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed 527bf02 on 8.3.x
    Issue #2706405 by heddn, edysmp, davidparedes21, mikeryan, phenaproxima...

  • catch committed 6fa3105 on 8.2.x
    Issue #2706405 by heddn, edysmp, davidparedes21, mikeryan, phenaproxima...

Status: Fixed » Closed (fixed)

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