Problem/Motivation

The d6_block migration attempts to migrate all records from the D6 blocks table, ordered by block ID (bid). When multiple themes are enabled on a site, then the same block can be listed multiple times in this table. For example, looking at the user login block (module = user, delta = 0), there may be several records in the D6 blocks table for this one block: http://note.io/1ioysQj

Keeping in mind that the blocks table has no knowledge of the current default theme on the D6 site, when the d6_block migration runs, it first migrates bid=1 from the screenshot above. This is the user login block placement in the Garland theme. This is migrated into D8 with the config name "block.block.user" (http://note.io/1ioz8Fj) and with an entry in the migrate_map_d6_block table (http://note.io/1iozfkf).

The next D6 block record to be migrated is bid=2, the user's "system_menu_block:tools" block (module = user, delta = 1). Again, this appears to be migrated into config properly with a name of "block.block.user_1" and a reasonable migrate_map_d6_block table record (http://note.io/1iozVWO).

Issue 1

Continuing, this appears to all work fine until we get to D6 block table records with: module = block (custom blocks): http://note.io/1ioAos3. The first one (bid=10) is migrated into D8 with a config name of "block.block.block" and a reasonable entry in migrate_map_d6_block (http://note.io/1ioAEHq). It is with the second custom block (bid=11) where we see things start to go sideways. There is no config entry created for the second block (leading me to think that there is an issue deduping custom block ids), but the migrate_map_d6_block table record again looks sound (http://note.io/1ioBdku).

Issue 2

Continuing with the block table migration, looking at bid=13, again, we're looking at a user login block (module = user, delta = 0), but for a different theme (chameleon). When the migration runs, it sees that we've already migrated a block with "sourceid1 = user" and "sourceid2 = 0", therefore it doesn't migrate it. But, as I mentioned earlier, the blocks table has no knowledge of the current default theme, so we may be ignoring block records for the current default theme that should be migrated.

Proposed resolution

Issue 1: fix the dedupe bug so that custom blocks are deduped properly (block, block_1, block_2, etc...)

Issue 2: this one is a little trickier and I think there are two options:

  1. Add a new sourceid to the migration, the theme name. I think this will also mean that we need to modify the config name of migrated blocks to something like: block.block.theme_name.block, block.block.theme_name.block_1, etc... (although I see config names like block.block.bartik_tools and block.block.bartik_help in the D8 config table, so maybe that's the way to go).
  2. Modify the d6_block query to only migrate blocks from the current default and admin themes. This will be a bit of a downer for sites with multiple active themes, but I think that's a pretty small percentage.

Remaining tasks

Find and fix the dedupe bug.
Determine which is the best solution for the multiple source theme issue and implement.

User interface changes

None.

API changes

Hopefully, none.

CommentFileSizeAuthor
#24 interdiff.txt717 byteschx
#24 2250429_21.patch17.53 KBchx
#21 dedupe-2250429-21.patch17.6 KBultimike
#18 dedupe-2250429-18.patch17.06 KBultimike
#16 dedupe-2250429-15.patch16.52 KBultimike
#12 dedupe-2250429-12.patch13.58 KBultimike
#8 dedupe-2250429-8.patch5.94 KBultimike
#5 dedupe-2250429-5.patch3.22 KBultimike
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented

I am a little confused about the problems outlined -- what doesn't get migrated , the blocks table or the boxes table? When migrating the blocks table (the block placement) into block config entities we do not use the delta at all; we use module as prefix (but honestly we could just use dancing_pink_elephant for a common prefix, it doesn't matter) and then dedupe. When migrating the boxes table into the custom block content entity then we are using the boxes.bid as an id which is equal to the delta in the blocks table, that's true. Not quite what doesn't work here. I suspect it's the former. Is the dedupe somehow broken? I will try to find the time to test this more; but if you get there first, DedupeEntity::exists is what needs debug.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

It's the blocks table that isn't migrating properly, and yes, I think the issue is with the dedupe function.

Here's another way of looking at it - when a custom block is created in D8, its "Block description" (and therefore its machine name) must be unique. For example, if the block description is "D8 custom block", then its config entry has the name "block.block.d8customblock".

When custom blocks block table records are migrated from D6 to D8, their config name is always "block.block.block". So, I'm guessing that the first one gets in okay, but then all the rest don't (because they have the same name).

This behavior is really easy to see with the patch from https://drupal.org/node/2248407 and test migrating at least two custom D6 blocks.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Issue summary: View changes

Ok - I did another round of (much more methodical) testing, and I think I've narrowed it down to 2 specific issues. I've updated the issue summary with the relevant info.

Thanks,
-mike

chx’s picture

chx CreditAttribution: chx commented

Issue #2 is easy to fix: the source getIds needs to contain theme, you are right.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Active » Needs review
FileSize
dedupe-2250429-5.patch3.22 KB

The attached patch should take care of both issues. Thanks to chx for all his guidance on this!

-mike

Status: Needs review » Needs work

The last submitted patch, 5: dedupe-2250429-5.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented

Wow! Someone wrote a unit test for DedupeEntity? Apparently we also need an integration test that checks for duplicated values. Block looks like a good place. To fix the unit test, remove TestDedupeEntity and create a mock of the QueryFactoryInterface which returns the $entity_query for every get call:

$entity_query_factory = $this->getMock('Drupal\Core\Entity\Query\QueryFactoryInterface');
$entity_query_factory->expects($this->any())
  ->method('get')
  ->will($this->returnValue($entity_query))
$plugin = new DedupeEntity($configuration, 'dedupe_entity', array(), $this->getMigration(), $entity_query_factory);

Note the __construct method doesn't pass the migration to the parent which it should... which finds us a new bug -- the ProcessPluginBase doesn't store $this->migration which it totally should. But that's a different issue.

And please please flesh poor block test out a bit. It's my fault that test is so nothing -- but getting block to *pass* was such arduous work I left it like that.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
FileSize
dedupe-2250429-8.patch5.94 KB

Patch updated with a working dedupe test.

Updating the block test should be a new issue, correct?

Thanks,
-mike

chx’s picture

chx CreditAttribution: chx commented

I think it can be very well be here; after all we just gotten the tests not break but didn't actually test the new things work. Thanks!

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

Looking at the current tests, here's what I see:

Drupal\migrate_drupal\Tests\d6\MigrateBlockTest

Currently creates 3 D8 blocks (including 1 custom block), then migrates 10 blocks (including one custom block) from a dump file. All migrated blocks are assigned to a single source theme (garland). The lone assertion is that after the migration, we have at least 1 block in D8 (I'm pretty sure I'm reading it correctly).

I think we should increase the number of both D8 and D6 blocks in setUp(), including multiple custom blocks (to test the deduping). I'll also need to figure out which additional assertions to add.

We can add additional tests for the block theme/region mapping as part of this patch: https://drupal.org/node/2248407

Drupal\migrate_drupal\Tests\d6\MigrateCustomBlockTest

Currently migrates 1 custom block from a dump file. Has multiple assertions for all the relevant block data.

I think we should increase the number of both D6 custom blocks to be migrated in setUp(). We can probably repeat the same (or similar assertions).

Does this sound about right? Will this cover what we need?

Thanks,
-mike

chx’s picture

chx CreditAttribution: chx commented

I think MigrateBlockTest is the one that needs help; MigrateCustomBlockTest is not affected. But yes, what I was talking about is the sad lonely assert in MigrateBlockTest.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
dedupe-2250429-12.patch13.58 KB

Updated the patch with a more robust test.

-mike

Status: Needs review » Needs work

The last submitted patch, 12: dedupe-2250429-12.patch, failed testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

Hmmm - I tried running the MigrateDrupal6Test in an up-to-date Drupal 8 environment (without this patch applied) and it failed pretty hard (http://note.io/1n2frVS).

Errors appear to be related to taxonomy term node, taxonomy term node revisions, url alias, user profile entity form display, vocabulary entity display, vocabulary entity form display, and vocabulary field instance migrations.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

My tests in comment 14 were run from the UI.

Based on chx's suggestion, I re-ran the tests via the CLI:

  1. MigrateDrupal6Test on 8.x, without the patch in #12 applied: passed
  2. MigrateBlockTest on 8.x, with the patch in #12 applied: passed
  3. MigrateDrupal6Test on 8.x, with the patch in #12 applied: failed

chx helped me understand that the MigrateDrupal6Test didn't run the setup() methods of the various tests, instead it just loads all the dump files and uses those as a giant combined setup (so to speak). Therefore, the custom blocks defined in core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6Box.php need to be in-sync with the blocks in core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6Block.php. I went ahead and made this change, then ran the MigrateBlockTest and MigrateCustomBlockTest separately (via the CLI and UI) to ensure that they both passed. They did.

chx asked me to codify the fact that custom blocks must be migrated before blocks - I did this by adding a migration dependency to d6_custom_block in the d6_block.yml. Unfortunately, the MigrateDrupal6Test still fails (see below for output).

I'm not really sure what to do from here. Help?!

Thanks,
-mike

~/Sites/drupal8 $ php core/scripts/run-tests.sh --url 'http://drupal8.dev/' --class 'Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test'

Drupal test run
---------------

Tests to be run:
  - Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test

Test run started:
  Friday, May 2, 2014 - 02:25

Test summary
------------


Fatal error: Call to a member function getConfigDependencyName() on a non-object in /Users/michael/Sites/drupal8/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php on line 378

Call Stack:
    0.0069     418304   1. {main}() /Users/michael/Sites/drupal8/core/scripts/run-tests.sh:0
    2.4236   19283256   2. simpletest_script_run_one_test() /Users/michael/Sites/drupal8/core/scripts/run-tests.sh:33
    2.4461   20636376   3. Drupal\simpletest\TestBase->run() /Users/michael/Sites/drupal8/core/scripts/run-tests.sh:699
  163.4004   56114760   4. Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test->testDrupal6() /Users/michael/Sites/drupal8/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php:868
  197.3430   58786368   5. Drupal\migrate\MigrateExecutable->import() /Users/michael/Sites/drupal8/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateDrupal6Test.php:310
  197.6631   58902792   6. Drupal\migrate\Plugin\migrate\destination\EntityConfigBase->import() /Users/michael/Sites/drupal8/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php:281
  197.6667   58907560   7. Drupal\Core\Entity\Entity->save() /Users/michael/Sites/drupal8/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityConfigBase.php:45
  197.6668   58907544   8. Drupal\Core\Config\Entity\ConfigEntityStorage->save() /Users/michael/Sites/drupal8/core/lib/Drupal/Core/Entity/Entity.php:303
  197.6669   58907632   9. Drupal\Core\Entity\EntityStorageBase->save() /Users/michael/Sites/drupal8/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:230
  197.6680   58908208  10. Drupal\field\Entity\FieldInstanceConfig->preSave() /Users/michael/Sites/drupal8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:390
  197.6682   58909080  11. Drupal\field\Entity\FieldInstanceConfig->calculateDependencies() /Users/michael/Sites/drupal8/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php:362

FATAL Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test: test runner returned a non-zero error code (255).
- Found database prefix 'simpletest999568' for test ID 29.
[02-May-2014 00:29:04 UTC] PHP Fatal error:  Call to a member function getConfigDependencyName() on a non-object in /Users/michael/Sites/drupal8/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php on line 378
[02-May-2014 00:29:04 UTC] PHP Stack trace:
[02-May-2014 00:29:04 UTC] PHP   1. {main}() /Users/michael/Sites/drupal8/core/scripts/run-tests.sh:0
[02-May-2014 00:29:04 UTC] PHP   2. simpletest_script_run_one_test() /Users/michael/Sites/drupal8/core/scripts/run-tests.sh:33
[02-May-2014 00:29:04 UTC] PHP   3. Drupal\simpletest\TestBase->run() /Users/michael/Sites/drupal8/core/scripts/run-tests.sh:699
[02-May-2014 00:29:04 UTC] PHP   4. Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test->testDrupal6() /Users/michael/Sites/drupal8/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php:868
[02-May-2014 00:29:04 UTC] PHP   5. Drupal\migrate\MigrateExecutable->import() /Users/michael/Sites/drupal8/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateDrupal6Test.php:310
[02-May-2014 00:29:04 UTC] PHP   6. Drupal\migrate\Plugin\migrate\destination\EntityConfigBase->import() /Users/michael/Sites/drupal8/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php:281
[02-May-2014 00:29:04 UTC] PHP   7. Drupal\Core\Entity\Entity->save() /Users/michael/Sites/drupal8/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/destination/EntityConfigBase.php:45
[02-May-2014 00:29:04 UTC] PHP   8. Drupal\Core\Config\Entity\ConfigEntityStorage->save() /Users/michael/Sites/drupal8/core/lib/Drupal/Core/Entity/Entity.php:303
[02-May-2014 00:29:04 UTC] PHP   9. Drupal\Core\Entity\EntityStorageBase->save() /Users/michael/Sites/drupal8/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:230
[02-May-2014 00:29:04 UTC] PHP  10. Drupal\field\Entity\FieldInstanceConfig->preSave() /Users/michael/Sites/drupal8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:390
[02-May-2014 00:29:04 UTC] PHP  11. Drupal\field\Entity\FieldInstanceConfig->calculateDependencies() /Users/michael/Sites/drupal8/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php:362

- Removed test site directory.
- Removed 183 leftover tables.

Test run duration: 3 min 20 sec
ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
dedupe-2250429-15.patch16.52 KB

Updated patch with the new custom block stuff I mentioned in the previous comment.

-mike

chx’s picture

chx CreditAttribution: chx commented

tl;dr: add d6_node_type to the migration dependencies of d6_upload_field_instance

Here's how to find this one out:

  1. put a breakpoint on $this->addDependency('entity', $bundle_entity->getConfigDependencyName());
    with the condition !is_object($bundle_entity). Go into the MigrateExecutable in the backtrace , open the migration entity belonging to it so now know the migration running is d6_upload_field_instance.
  2. So this tries to create the field instance node.page.upload but can not find the entity of type node_type with id page.
  3. MigrateUploadInstanceTest::setUp creates the page node type. But MigrateDrupal6Test does not run setUp of individual test but rather depends on previous tests to do it. First thing to check: is the dependency in order? Well, dependency on what?
  4. Where do node types come from?
    grep entity:node_type core/modules/migrate_drupal/config/install/*
core/modules/migrate_drupal/config/install/migrate.migration.d6_node_type.yml:  plugin: entity:node_type

    Nothing else. That makes it easy.

  5. Now we check d6_upload_field_instance migrate dependencies recursively and quickly find that it depends on d6_upload_field but d6_upload_field has no migration_dependencies. That's the bug. We forgot a dependency, your new dependency have shuffled the sort enough that the previous accidental order is no longer in effect
ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
FileSize
dedupe-2250429-18.patch17.06 KB

chx,

Whoa - that's some strong kung-fu. Your explanation make sense, but I'm still a little ways away from being able to figure that out on my own. Thanks for the assistance.

Updated patch attached.

Thanks,
-mike

Status: Needs review » Needs work

The last submitted patch, 18: dedupe-2250429-18.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented

Ah yes! Adding a migration_dependency requires patching the id mappings in setUp of the relevant test. MigrateFieldInstanceTest has an example for d6_node_type mappings. Beyond that, we only have a fail that seems relevant to the issue.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
dedupe-2250429-21.patch17.6 KB

Let's try this again - I fixed an issue with the custom block DB dump. I just ran all the migrate_drupal" tests locally and they all passed. Fingers crossed...

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review

The last submitted patch, 16: dedupe-2250429-15.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community
FileSize
2250429_21.patch17.53 KB
interdiff.txt717 bytes

Amazing hard work, thanks so much! I have removed one too many copypasted lines (this test only creates page and story, see the entity_create just after the prepareIdMappings call) otherwise it's good. In a followup perhaps we should add a test for an aggregator block too but that's definitely not this issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2250429_21.patch, failed testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

24: 2250429_21.patch queued for re-testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC after a bot fluke.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed b45b871 and pushed to 8.x. Thanks!

  • Commit b45b871 on 8.x by alexpott: 
    Issue #2250429 by ultimike, chx: D6_block migration not migrating all...

Status: Fixed » Closed (fixed)

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