Private files need a separate migration from public files, because their source path will differ. Haven't looked too closely, but this probably requires core support - like, an option on the file source plugin to select only public or only private files.

The hard part here is the UX - from comment #3 below:

In Drupal 6, the files directory is either all public or private, so there's only one file directory configuration to worry about. In Drupal 7 you can have a public file directory and and private file directory (and the latter isn't necessarily in the docroot), so we need to configure two file migration processes, one for each. In our current workflow, we don't know whether we're dealing with a D6 or D7 site (and thus whether we need to prompt for a private file directory) until submitting the credential form. How do we deal with this?

  1. Instead of auto-detecting the source version, prompt for it on the credentials form, and use ajax to show/hide the private directory prompt (submitting the form would still detect the version and throw an error if it didn't match the selected version).
  2. Have a separate page after the credentials form for file configuration, which can easily have one or two file directory prompts as appropriate. And perhaps other version-specific prompts as well...
  3. Other ideas?
Files: 
CommentFileSizeAuthor
#68 interdiff-66-68.txt2.19 KBJo Fitzgerald
#68 2505283-68.patch22.85 KBJo Fitzgerald
#66 interdiff.txt8.4 KBquietone
#66 2505283-66.patch22.78 KBquietone
#63 interdiff.txt539 bytesquietone
#63 2505283-63.patch26.85 KBquietone
#61 2505283-61.patch26.79 KBquietone
#59 2505283-59.patch26.82 KBquietone
#55 D7.png47.85 KBquietone
#55 D6.png36.65 KBquietone
#52 interdiff.txt539 bytesquietone
#52 2505283-52.patch26.86 KBquietone
#50 interdiff-48-50.txt706 bytesJo Fitzgerald
#50 2505283-50.patch26.8 KBJo Fitzgerald
#48 interdiff-43-48.txt2.68 KBJo Fitzgerald
#48 2505283-48.patch26.7 KBJo Fitzgerald
#43 interdiff.txt17.38 KBquietone
#43 2505283-43.patch26.58 KBquietone
#42 interdiff.txt586 bytesquietone
#42 2505283-42.patch8.98 KBquietone
#40 interdiff.txt1.01 KBquietone
#40 2505283-40.patch8.98 KBquietone
#38 interdiff.txt786 bytesquietone
#38 2505283-38.patch8.64 KBquietone
#36 interdiff.txt4.2 KBquietone
#36 2505283-36.patch8.57 KBquietone
#32 2505283-32.patch7 KBquietone

Comments

mikeryan’s picture

mikeryan’s picture

Issue tags: +Barcelona2015, +Novice

Tagged for Barcelona sprinting. Note that D6 has a global public/private switch, so only needs one file source to run, but D7 has separate public and private file directories (and private files aren't necessarily in the docroot), so the UI should only prompt for a private file directories if the source site is D7, and is configured for private files. Not quite sure how "novice" this is...

mikeryan’s picture

I've added the following to the design doc:

So, private files... In Drupal 6, the files directory is either all public or private, so there's only one file directory configuration to worry about. In Drupal 7 you can have a public file directory and and private file directory (and the latter isn't necessarily in the docroot), so we need to configure two file migration processes, one for each. In our current workflow, we don't know whether we're dealing with a D6 or D7 site (and thus whether we need to prompt for a private file directory) until submitting the credential form. How do we deal with this?

  1. Instead of auto-detecting the source version, prompt for it on the credentials form, and use ajax to show/hide the private directory prompt (submitting the form would still detect the version and throw an error if it didn't match the selected version).
  2. Have a separate page after the credentials form for file configuration, which can easily have one or two file directory prompts as appropriate. And perhaps other version-specific prompts as well...
  3. Other ideas?
mikeryan’s picture

A first pass at implementing private file handling in the drush command (with added option --legacy-root-private), which works with the core patch at #2547125: D7 file migration should allow selecting public/private/all files. This stage is basically just to demonstrate that the core patch works for reals.

mikeryan’s picture

Just a note on that patch - the big hole is that for any references to the private files (i.e., file fields) to work, the private file migration needs to be referenced by the migration process plugin. So, we'll need to rewrite other migrations to add it there, as well as to migrate_dependencies...

mikeryan’s picture

FileSize
5.78 KB

Here's the patch with some ugly code to rewrite references to d7_file. Haven't done anything with the UI, not quite sure yet where/when we should prompt for the private files.

abhishek-anand’s picture

Status: Postponed » Needs work
malavya’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: handle_import_of-2505283-6.patch, failed testing.

xjm’s picture

Project: Migrate Upgrade » Drupal core
Version: 8.x-1.x-dev » 8.1.x-dev
Component: Code » migration system
AjitS’s picture

Issue tags: -Novice

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.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

xjm’s picture

Priority: Normal » Major
Issue tags: +Migrate critical

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

xjm’s picture

Status: Needs work » Postponed

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

Ryan Weal’s picture

Ryan Weal’s picture

Issue tags: +neworleans2016

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.

mikeryan’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Postponed » Needs work

EntityFile patch is in, this can proceed.

chriscalip’s picture

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

mikeryan’s picture

Issue tags: +Needs reroll

The last patch is ancient, from before upgrade UI was folded into core, it needs to be rerolled against the current core UI.

nlisgo’s picture

Assigned: Unassigned » nlisgo
Issue tags: +Dublin2016

Going to attempt a reroll.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Issue tags: -Dublin2016
penyaskito’s picture

Issue tags: -Needs reroll

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

penyaskito’s picture

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

chriscalip’s picture

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

I guess it doesnt matter, comment withdrawn.

penyaskito’s picture

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.

kekkis’s picture

Issue tags: +Needs reroll

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.

mikeryan’s picture

Issue summary: View changes
Issue tags: +D8UX usability

This patch was begun when the upgrade UI was still in the migrate_upgrade contrib module, and thus covered both the UI (now in core) and the drush command (still in contrib). The MigrationCreationTrait needs to be redone against the core MigrationConfigurationTrait - then, the equivalent of what was being done here for the drush command needs to be done in the core UI.

mikeryan’s picture

The hard part here is the UX - from comment #3 above:

In Drupal 6, the files directory is either all public or private, so there's only one file directory configuration to worry about. In Drupal 7 you can have a public file directory and and private file directory (and the latter isn't necessarily in the docroot), so we need to configure two file migration processes, one for each. In our current workflow, we don't know whether we're dealing with a D6 or D7 site (and thus whether we need to prompt for a private file directory) until submitting the credential form. How do we deal with this?

  1. Instead of auto-detecting the source version, prompt for it on the credentials form, and use ajax to show/hide the private directory prompt (submitting the form would still detect the version and throw an error if it didn't match the selected version).
  2. Have a separate page after the credentials form for file configuration, which can easily have one or two file directory prompts as appropriate. And perhaps other version-specific prompts as well...
  3. Other ideas?
mikeryan’s picture

Issue summary: View changes

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

quietone’s picture

Status: Needs work » Needs review
FileSize
7 KB

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

mikeryan’s picture

Assigned: Unassigned » mikeryan
anish.a’s picture

mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -854,6 +854,15 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
    +      '#default_value' => 6,
    

    There are many more Drupal 7 than Drupal 6 sites out there, I would make Drupal 7 the default.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -854,6 +854,15 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
    +      '#options' => [6 => t('Drupal 6'), 7 => t('Drupal 7')],
    

    s/t/$this->t/

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -913,6 +922,21 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
           '#description' => $this->t('To import files from your current Drupal site, enter a local file directory containing your site (e.g. /var/www/docroot), or your site address (for example http://example.com).'),
    

    It'd be nice if for Drupal 7 this would change to "To import *public* files..." (and the title change to "Public file directory").

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -913,6 +922,21 @@ public function buildCredentialForm(array $form, FormStateInterface $form_state)
    +      '#description' => $this->t('To import private files from your current Drupal site, enter a local file directory containing your site (e.g. /var/www/docroot), or your site address (for example http://example.com).'),
    

    Site address won't work for private files (because they're private!) - only a local file directory will work.

quietone’s picture

Status: Needs work » Needs review
FileSize
8.57 KB
4.2 KB

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.

Status: Needs review » Needs work

The last submitted patch, 36: 2505283-36.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
8.64 KB
786 bytes

Wrong patch. Try this one.

Status: Needs review » Needs work

The last submitted patch, 38: 2505283-38.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
8.98 KB
1.01 KB

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

Status: Needs review » Needs work

The last submitted patch, 40: 2505283-40.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
8.98 KB
586 bytes

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

quietone’s picture

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.

Pavan B S’s picture

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

The last submitted patch, 43: 2505283-43.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 44: 2505283-45.patch, failed testing.

quietone’s picture

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
26.7 KB
2.68 KB

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.

Status: Needs review » Needs work

The last submitted patch, 48: 2505283-48.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
26.8 KB
706 bytes

Let's make sure the directory exists first.

Status: Needs review » Needs work

The last submitted patch, 50: 2505283-50.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
26.86 KB
539 bytes

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

Jo Fitzgerald’s picture

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

mikeryan’s picture

Issue tags: +Migrate UI
quietone’s picture

FileSize
36.65 KB
47.85 KB

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.

heddn’s picture

Assigned: Unassigned » mikeryan

Assigning to mikeryan to review.

Status: Needs review » Needs work

The last submitted patch, 52: 2505283-52.patch, failed testing.

mikeryan’s picture

Assigned: mikeryan » Unassigned
Issue tags: +Needs reroll

Patch no longer applies, so not tested. However, I did do a quick code review:

  1. +++ b/core/modules/file/migration_templates/d7_file_private.yml
    @@ -0,0 +1,42 @@
    +    source_file_private_path: ''
    

    Ideally, we would use the same constant name, source_base_path, for both public and private migrations in the migration itself (of course, the form field name needs to differ). Since there's only one path being specified in the migration, we don't need a different name.

  2. +++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
    @@ -83,8 +83,12 @@ public function prepareRow(Row $row) {
    -    // the source_base_path in order to make them all relative.
    -    $path = str_replace($this->configuration['constants']['source_base_path'], NULL, $path);
    

    As above, I think we can just keep this.

  3. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
    @@ -105,6 +105,7 @@ public static function run($initial_ids, $config, &$context) {
         if ($definition['destination']['plugin'] === 'entity:file') {
           // Make sure we have a single trailing slash.
           $configuration['source']['constants']['source_base_path'] = rtrim($config['source_base_path'], '/') . '/';
    +      $configuration['source']['constants']['source_private_file_path'] = rtrim($config['source_private_file_path'], '/') . '/';
    

    So, I would say that here we need to explicitly use the form's source_private_file_path when the migration ID is 'd7_file_private', and otherwise use source_base_path.

Thanks!

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
26.82 KB

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

Status: Needs review » Needs work

The last submitted patch, 59: 2505283-59.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
26.79 KB

The reroll without the typo.

Status: Needs review » Needs work

The last submitted patch, 61: 2505283-61.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
26.85 KB
539 bytes

And adjust the count.

mikeryan’s picture

Status: Needs review » Needs work

Needs work for #58.

quietone’s picture

Assigned: Unassigned » quietone

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

quietone’s picture

Assigned: quietone » Unassigned
Status: Needs work » Needs review
FileSize
22.78 KB
8.4 KB

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.

Status: Needs review » Needs work

The last submitted patch, 66: 2505283-66.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
22.85 KB
2.19 KB

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.

Status: Needs review » Needs work

The last submitted patch, 68: 2505283-68.patch, failed testing.