Migration issue

I am use Migrate Drupal UI for migration from Drupal 6 to Drupal 8 version. In Migrate Drupal UI I pick Drupal 6 and fill all fields correctly (included "Files directory"). "Files directory" was filled by path of real D6 directory with maximum permissions. But every time after launch files from D6 wasn't migrated.

Observations

Migrate Drupal UI uses different "Files directory" field for each version (D6 and D7):

    $form['source']['d6_source_base_path'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Files directory'),
      '#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).'),
      '#states' => [
        'visible' => [
          ':input[name="version"]' => ['value' => 6],
        ],
      ],
    ];

    $form['source']['source_base_path'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Public files directory'),
      '#description' => $this->t('To import public 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).'),
      '#states' => [
        'visible' => [
          ':input[name="version"]' => ['value' => 7],
        ],
      ],
    ];

After submitting Drupal uses "source_base_path" variable for directory, but ignore "d6_source_base_path" variable. And code doesn't have part, that copy data from "d6_source_base_path" to "source_base_path".

And if I tried to fill D7 "Public files directory" field (when I picked D7), after that switch to D6 and submitted alway it works correctly. It means that "d6_source_base_path" field is ignored.

Solution

I prefer add the next code in validation block:

    if($version == 6) $form_state->setValue('source_base_path', $form_state->getValue('d6_source_base_path'));

I attached patch file.

CommentFileSizeAuthor
#12 file_migration_from_d6-2907233-12.patch1.12 KBldavidsp
#9 2907233-9.patch2.09 KBquietone
#9 2907233-9-fail.patch980 bytesquietone
#5 2907233-5.patch3.21 KBquietone
#5 2907233-5-fail.patch2.07 KBquietone
migrate_drupal_ui_migrate_upgrade_form.patch657 byteslobodakyrylo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

madlobz created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Priority: Normal » Major
Issue tags: -File migration, -Drupal 8, -Drupal 6, -Migrate Drupal UI +migrate-d6-d8, +Migrate critical, +Migrate UI, +Needs tests

The code for different source_base_paths for D6 and D7 was added in #2505283: Handle import of private files. Early versions of the patch did get the source_base_path by checking the Drupal version but somehow that was removed in later patches. So, let's add the functionality back and then, of course, we need tests.

Although I haven't tried to reproduce the error it seems obvious from reading the code that this is a problem. Marking as migrate-critical since migrating from D6 from the UI should be a few easy clicks.

Updating the tags as well.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.3.7 » 8.4.x-dev
Priority: Major » Critical

Per discussion with @catch, we're promoting the "Migrate critical" issues to critical given the importance of a stable migration path at this point in the release cycle. (The framework and release managers will confirm this in a later triage for individual issues.)

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Active » Needs work 
+++ /core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php	2017-09-06 22:05:15.000000000 +0300
@@ -1052,6 +1052,9 @@
+        if($version == 6) $form_state->setValue('source_base_path', $form_state->getValue('d6_source_base_path'));

This if statement should be formatted differently.

And we need tests.

quietone’s picture

quietone CreditAttribution: quietone at Acro Commerce commented
Status: Needs work » Needs review
FileSize
2907233-5-fail.patch2.07 KB
2907233-5.patch3.21 KB

An attempt at a test and the fix.

The last submitted patch, 5: 2907233-5-fail.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 2907233-5.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Assigned: lobodakyrylo » quietone
Status: Needs work » Needs review
Issue tags: +Vienna2017

Working on this at the sprint.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
2907233-9-fail.patch980 bytes
2907233-9.patch2.09 KB

Try again. No interdiff because the patch is small.

The last submitted patch, migrate_drupal_ui_migrate_upgrade_form.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 9: 2907233-9-fail.patch, failed testing. View results

ldavidsp’s picture

ldavidsp CreditAttribution: ldavidsp at MTech, LLC commented
FileSize
file_migration_from_d6-2907233-12.patch1.12 KB
ldavidsp’s picture

ldavidsp CreditAttribution: ldavidsp at MTech, LLC commented
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Needs review » Reviewed & tested by the community
FileSize
2907233-9.patch2.09 KB

Re-uploading #9 as the last patch. There's tests and this fixes a fairly obvious regression. #12 doesn't go quite in the correct direction.

maxocub’s picture

maxocub CreditAttribution: maxocub for Acquia commented
Issue tags: -Needs tests

The patch looks great.

I was wondering why the tests were passing before and this bug was not discovered by them, but as we can see with the failing test patch, it's because the tests were specifically using source_base_path instead of d6_source_base_path, so that's why.

I think this is enough for test coverage, so RTBC (since it was green in #9)

The last submitted patch, 12: file_migration_from_d6-2907233-12.patch, failed testing. View results

maxocub’s picture

maxocub CreditAttribution: maxocub for Acquia commented
Status: Needs review » Reviewed & tested by the community

Wrong file failed testing, back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Reviewed this at DrupalCon Vienna with the Migrate team!

This fixes a long-standing bug in Migrate UI, and gets us one step closer to a stable Migration path yay!

The patch looks good, there's a failing test patch, AND @quietone demonstrated the patch on her laptop, so confirmed everything looks good.

Committed and pushed to 8.5.x and cherry-picked to 8.4.x.

  • webchick committed 8685e7c on 8.5.x 
    Issue #2907233 by quietone, madlobz, heddn, maxocub: File migration from...

  • webchick committed 942b0ed on 8.4.x 
    Issue #2907233 by quietone, madlobz, heddn, maxocub: File migration from...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Assigned: quietone » Unassigned