Handle import of private files

I think this is not a suitable candidate for the "novice" contributors as per https://www.drupal.org/core-mentoring/novice-tasks.
The process to this issue looks to be open-ended. The scope too is not defined clearly. Please correct if you feel otherwise.

Since private files are a core feature, we need to have a migration path for them, so this is a Migrate critical.

Postponing on #2695297: Refactor EntityFile and use process plugins instead per discussion with @alexpott, @weal, and @mikeryan.

I am confused by patch. I am unable to apply patch see @screenshot 2505283-unable-to-apply-patch.png
Also looks like this patch apply to contrib module drupal.org/project/migrate_upgrade

Going to attempt a reroll.

This has changed so much that I don't think we can reroll anything, so let's start from scratch.

Work in progress. I managed to do the part of the patch that should be applied to migrate_update. Still work in core needed.

hmm.. am i missing something here? migrate_upgrade is a contrib module.

I guess it doesnt matter, comment withdrawn.

What I tried in #24 is to generate another migration in getMigrations(), but at that point we don't have yet control over the settings we are passing.
Generating them as soon as we have the settings would mean duplicating code in the UpgradeForm and the Upgrade Drush Runner.

I'm wondering if we should generate the d7_files_private when we already have that info, but the user won't see a "d7_files_private" migration at all.

Patch at #24 is not valid since it's from a diff taken at module level, not Drupal root directory level. I'm unable to find the migrate_upgrade.drush.inc file in Drupal core at all, even after running composer install.

Making a start. This implements changes to the UI only, option #1.

Fixes for 1-4 in #35. There may be a better way to resolve #3. Don't know why but I was getting errors on d6_statistics_settings, so I changed that to the correct name.

Wrong patch. Try this one.

The form has a new field for the d6 source base path, now make sure MigrateUpgradeTest knows about it.

Well, that was a silly mistake. Edited out the setting of source_base_path.

Added a private file migration and test, using the existing file migration as a guide. But the private file migration test fails because the call to realpath returns 'private:' and not the realpath. How should the private file stream be setup for tests?

Also, changed d7_file to use the scheme configuration parameter to select only public files.

The patch doesn't use the new short array syntax of , made change to the short array. Applying the patch, please review.

@Pavan B S, thanks for helping out. However, the migrate test fixtures don't use the short array syntax. The first lines of the fixture are

// @codingStandardsIgnoreFile
/**
 * @file
 * A database agnostic dump for testing purposes.
 *
 * This file was generated by the Drupal 8.0 db-tools.php script.
 */

There is some more information at Generating database fixtures for D8 Migrate tests.

I'm pretty sure this is not the correct/final method, but after a lot of playing around I have at least got the tests to pass.

The register() method appears to be the way to enable the private stream (which means realpath() returns the values we need) - I obtained this concept from FileTestBase.

I'm not happy with:

+++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
@@ -84,7 +84,7 @@ public function prepareRow(Row $row) {
-      $path = str_replace($this->configuration['constants']['source_file_private_path'], NULL, $path);
+      $path = str_replace($this->configuration['constants']['source_file_private_path'], NULL, realpath($path));

but I suspect the str_replace() is pointless without it. Whatever we end up doing in this if() probably should also be applied to the "else".

The file system is now obtained from the container, rather than \Drupal::service() which feels right.

Hopefully this gets us a step closer.

I think we can remove the Needs Tests tag too.

Let's make sure the directory exists first.

@Jo Fitzgerald, thanks. Getting the file system from the container and the new register method makes sense. If this is the way to go these changes should be made in MigrateFileTest as well.

This patch changes changes the file entity count in MigrateUpgrade7Test to include the new private file migration.

I have opened a follow-up ticket for copying the changes from here to MigrateFileTest: #2860760: Match setup() functionality of MigrateFileTest with MigratePrivateFileTest

Came back to check for an error I thought I left in the patch. Good news, I didn't. So, yes, ready for review.
I decided to add some screenshots.

Assigning to mikeryan to review.

Only the reroll. Still need to respond to #58.

The reroll without the typo.

And adjust the count.

I thought I uploaded a patch for #58! Assigning to myself until I have time to go hunting for it.

Addressing #58. Unfortunately, this isn't passing all tests locally. But since PhpStorm has decided (for the Nth time) to stop debugging I'm going to take a long pause.

I took some queues from Drupal\Core\StreamWrapper\PublicStream::basePath() for setting file_private_path and using a virtual file system (vfs), similar to MigrateFileTest.

I also made a couple of corrections to d7_file_private.yml to bring it in line with d7_file.yml and $source['constants']['source_base_path'] = $fs->realpath('private://'); in the test setup.

If this needs any further changes then I expect they will need to be applied to the public equivalent too.

+++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
@@ -130,6 +130,7 @@ public function testMigrateUpgrade() {
     $edit = [
       $driver => $connection_options,
       'source_base_path' => $this->getSourceBasePath(),
+      'source_private_file_path' => $this->getSourceBasePath(),
     ];

This should be 'source_base_path', no?

No, because that will replicate the preceding line (but I can't be any more helpful than that, sorry).

I've spent all day on this and I've got nowhere.

In MigrateUpgrade6Test, $user = User::load(2); returns null in both patches since #63 (#66 & #68), but I can't fathom why. I assume the User migration is failing due to changes in importing files. Running WebTestBase tests takes forever and is awkward to debug.

Can someone else take a look at this? I'd love to know what is causing the problem.

Good news, MigrateUpgradeTestBase no longer extends WTB, but BTB instead, so this will be easier to debug.
Here's a re-roll.

I think this should take care of problem mentioned in #72.
We needed to specify the source Drupal version, the newly added radio button.

Now only the private files migration are still failing.

This looks real good, but I have a few requests...

1. +++ b/core/modules/file/migration_templates/d7_file_private.yml
   @@ -0,0 +1,42 @@
   +  # filesize is dynamically computed when file entities are saved, so there is
   +  # no point in migrating it.
   +  # filesize: filesize
   

   We can just get rid of this. Commented-out code is non-great :)

2. +++ b/core/modules/file/tests/src/Kernel/Migrate/d7/MigratePrivateFileTest.php
   @@ -0,0 +1,95 @@
   +  public static $modules = ['file'];
   

   Needs @inheritdoc.

3. +++ b/core/modules/file/tests/src/Kernel/Migrate/d7/MigratePrivateFileTest.php
   @@ -0,0 +1,95 @@
   +    $this->assertTrue($file instanceof FileInterface);
   

   This should be assertInstanceOf().

4. +++ b/core/modules/file/tests/src/Kernel/Migrate/d7/MigratePrivateFileTest.php
   @@ -0,0 +1,95 @@
   +    $this->assertTrue(file_exists($uri));
   

   We might be able to use assertFileExists() here.

5. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -938,10 +951,47 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
   +      '#states' => [
   +        'invisible' => [
   +          ':input[name="version"]' => ['value' => 7],
   +        ],
   +        'visible' => [
   +          ':input[name="version"]' => ['value' => 6],
   +        ],
   +      ],
   

   These states are redundant. We only need to specify either invisible or visible, but not both.

6. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -938,10 +951,47 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
   +      '#states' => [
   +        'invisible' => [
   +          ':input[name="version"]' => ['value' => 6],
   +        ],
   +        'visible' => [
   +          ':input[name="version"]' => ['value' => 7],
   +        ],
   +      ],
   

   Same here.

7. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -938,10 +951,47 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
   +      '#states' => [
   +        'invisible' => [
   +          ':input[name="version"]' => ['value' => 6],
   +        ],
   +        'visible' => [
   +          ':input[name="version"]' => ['value' => 7],
   +        ],
   +      ],
   

   And here.

Corrections from #77.

This should be targeted against 8.4.x.

Looks great to me!

Brought to you by the smart people at DrupalCamp Montréal!

Also, as per discussion with @xjm, this can be targeted against 8.3.x. My bad on that one.

Yep, to clarify #81, Migrate Drupal is still in alpha, and although the migrations technically live in the stable module that provides them, we've been treating them as covered by the experimental module backport exception. (Reference: https://www.drupal.org/core/d8-allowed-changes#experimental)

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