Problem/Motivation

Tried a vanilla D6 migration to 8.1.x. No CCK, nothing.

$ drush @d8.local migrate-upgrade --legacy-db-url=mysql://root:root@localhost:3306/d6 --legacy-root=http://d6.local

Returned:

(snipped)
Upgrading d6_node:page
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd6.content_node_field' doesn't exist: SELECT COUNT(*) AS expression         [error]
FROM 
(SELECT DISTINCT cnf.field_name AS field_name, cnf.type AS type, cnf.global_settings AS global_settings, cnf.required AS required,
cnf.multiple AS multiple, cnf.db_storage AS db_storage, cnf.module AS module, cnf.db_columns AS db_columns, cnf.active AS active,
cnf.locked AS locked, 1 AS expression
FROM 
{content_node_field} cnf
INNER JOIN {content_node_field_instance} cnfi ON cnfi.field_name = cnf.field_name) subquery; Array
(
)

Which I *thought* had been fixed as part of #2561697: Migration should not choke when the content_node_field table isn't available. Enabling CCK's content module fixes the issue, but we shouldn't be assuming it's there.

Proposed resolution

Make sure CCK's content module isn't required for a default migration to run successfully

Remaining tasks

There was possibly a regression after #2561697: Migration should not choke when the content_node_field table isn't available - Review and or git bisect and fix what needs to be fixed.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anavarre created an issue. See original summary.

anavarre’s picture

Issue tags: +migrate-d6-d8
catch’s picture

Issue tags: +Needs tests
Anonymous’s picture

Issue tags: +neworleans2016
douggreen’s picture

vasi’s picture

I tried this with both migrate_drupal_ui and migrate_upgrade. Strangely, it only fails with migrate_upgrade!

vasi’s picture

So this is interesting! I've configured migrations, and written this:

function get_requirements($migration) {
  $reflection = new ReflectionClass($migration);
  $property = $reflection->getProperty('requirements');
  $property->setAccessible(TRUE);
  $requirements = $property->getValue($migration);
  return array_keys($requirements);
}

$migration_id = 'd6_node:page';
$manager = \Drupal::service('plugin.manager.migration');

$migration = $manager->createInstance($migration_id);
var_dump(get_requirements($migration));

$migrations = $manager->createInstancesByTag('Drupal 6');
$migration = $migrations[$migration_id];
var_dump(get_requirements($migration));

This yields the following:

array(4) {
  [0]=>
  string(7) "d6_user"
  [1]=>
  string(12) "d6_node_type"
  [2]=>
  string(16) "d6_node_settings"
  [3]=>
  string(16) "d6_filter_format"
}
array(18) {
  [0]=>
  string(7) "d6_user"
  [1]=>
  string(12) "d6_user_role"
  [2]=>
  string(16) "d6_filter_format"
  [3]=>
  string(20) "d6_user_picture_file"
  [4]=>
  string(7) "d6_file"
  [5]=>
  string(27) "user_picture_entity_display"
  [6]=>
  string(27) "user_picture_field_instance"
  [7]=>
  string(18) "user_picture_field"
  [8]=>
  string(32) "user_picture_entity_form_display"
  [9]=>
  string(12) "d6_node_type"
  [10]=>
  string(16) "d6_node_settings"
  [11]=>
  string(33) "d6_field_instance_widget_settings"
  [12]=>
  string(17) "d6_field_instance"
  [13]=>
  string(8) "d6_field"
  [14]=>
  string(27) "d6_field_formatter_settings"
  [15]=>
  string(13) "d6_view_modes"
  [16]=>
  string(24) "d6_upload_field_instance"
  [17]=>
  string(15) "d6_upload_field"
}

Why are they different????

vasi’s picture

Status: Active » Needs review
FileSize
5.62 KB
6.84 KB

This seems to be happening because of some code in MigrationPluginManager:: buildDependencyMigration():

foreach ($migrations as $migration) {
  // ...
  $migration_dependencies = $migration->getMigrationDependencies();
  // ...
}
// ...
if (!empty($migration_dependencies['optional'])) {
  $required_dependency_graph = (new Graph($required_dependency_graph))->searchAndSort();
}
else {
  $required_dependency_graph = $dependency_graph;
}

Our condition at the end depends not on some global property, but on the dependencies of the last migration in the list being passed in. If the last migration has no optional dependencies, then all optional dependencies end up converted to required ones!

In our case, that means the d6_field migration is added to the d6_node:page one, even though it's optional. It looks like the UI first calculates all the migrations and their order, but then when running them loads them one at a time—so it doesn't encounter this problem.

I've added a test to reproduce this misbehaviour of buildDependencyMigration(), as well as just generally testing it, since it's complex and has no tests. This should fail.

Also added a fix.

The last submitted patch, 8: 2710133-8.fails_.patch, failed testing.

catch’s picture

Priority: Normal » Major

Good find and simple fix.

Bumping to major for now.

mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -176,10 +177,11 @@ public function buildDependencyMigration(array $migrations, array $dynamic_ids)
    +        $have_optional = TRUE;
    

    Not showing in the patch, but this is inside an isset($migration_dependencies['optional']) - it could be hit on an empty 'optional' array. Perhaps change the isset to !empty?

  2. +++ b/core/modules/migrate/tests/src/Unit/MigrationPluginManagerTest.php
    @@ -0,0 +1,210 @@
    +  /**
    

    Blank line at start of class.

vasi’s picture

Ok, those should be fixed. I can't add a test for the empty optionals situation, because the only harm it caused was disabling an optimization.

Also changed the test to make sure that ->set() is only called once.

Status: Needs review » Needs work

The last submitted patch, 12: 2710133-12.patch, failed testing.

mikeryan’s picture

Issue tags: -Needs tests
mikeryan’s picture

Status: Needs work » Reviewed & tested by the community

Looks good to me!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1efd63e and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 5e61925 on 8.2.x
    Issue #2710133 by vasi, mikeryan: Follow up to #2561697 - Migration...

  • alexpott committed 1efd63e on 8.1.x
    Issue #2710133 by vasi, mikeryan: Follow up to #2561697 - Migration...
mkalkbrenner’s picture

This commit introduced a fatal error when running phpunit. See #2725755: Fatal error: Cannot redeclare class Drupal\Tests\migrate\Unit\TestMigration for details.

Status: Fixed » Closed (fixed)

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