Problem/Motivation

We can't reliably use idMap to determine if a migration has successfully run. We currently use the idMap to check completion and it falls over when migrations have a hard dependency on another migration which has completed but not actually migrated any rows. For example #2233901: d6_node_revision sometimes doesn't have anything to migrate, then fails checkRequirements?

Proposed resolution

Store the result of a migration in the database.

Remaining tasks

Write the patch.

User interface changes

n/a

API changes

Unlikely that we remove anything but possibly a few API additions.

CommentFileSizeAuthor
#15 interdiff.txt457 bytesbenjy
#15 2314289-15.patch10.09 KBbenjy
#11 interdiff.txt1.32 KBbenjy
#11 2314289-11.patch9.64 KBbenjy
#9 interdiff.txt3.72 KBbenjy
#9 2314289-9.patch9.48 KBbenjy
#8 interdiff.txt2.57 KBbenjy
#8 2314289-8.patch6.7 KBbenjy
#6 interdiff.txt585 bytesbenjy
#6 2314289-6.patch4.7 KBbenjy
#3 interdiff.txt3.55 KBbenjy
#3 interdiff2.txt695 bytesbenjy
#3 2314289-3.patch4.13 KBbenjy
#2 2289503-1.patch4.17 KBbenjy

Comments

benjy’s picture

benjy commented
Issue summary: View changes
benjy’s picture

benjy commented
StatusFileSize
new 2289503-1.patch4.17 KB

A POC attached.

Open to suggestions on how we should store the data. I keyed it by [migrate][migration_id][status] so we can add extra data later but i'm open to suggestions.

      // @todo, this certainly doesn't belong here!!
      $migration->setMigrationStatus(MigrationInterface::RESULT_COMPLETED);

This doesn't belong in prepareIdMappings() but I put it there to test as prepareIdMappings() is called at the right time for this to happen and it saved me updating every test. Not sure if we should refactor and have this in MigrationTestBase::prepare() or whether it should just be it's own method that is called from every tests with dependencies?

benjy’s picture

benjy commented
Status: Active » Needs review
StatusFileSize
new 2314289-3.patch4.13 KB
new interdiff2.txt695 bytes
new interdiff.txt3.55 KB
  1. Renamed setMigrationStatus() to setMigrationResult()
  2. Added a @TODO for renaming of prepareIdMappings() which lets us leave $migration->setMigrationResult() where it is. #2315489: Renamed prepareIdMappings() in MigrateTestBase
  3. Refactored to use the keyValue service directly rather than the State API.
benjy’s picture

benjy commented

I was looking at ways to add a test and I couldn't decide on the best approach. Unit testing won't be easy since Migration:;checkRequirements() uses \Drupal::entityManager(). We could refactor that but we don't currently inject anything into the Migration entity.

Otherwise, a Simpletest test for this would probably be a simple migration. We don't currently have any migrations that are just for testing but that might be something we should introduce?

chx’s picture

chx commented

I would think since we changed the dependency checker we finally could use dependencies for real and thus test...? Ie if a migration wasnt run a dependent one won't.

benjy’s picture

benjy commented
StatusFileSize
new 2314289-6.patch4.7 KB
new interdiff.txt585 bytes

Uploading patch to test this new functionality. There could be other migrations that need dependencies moving from optional to required and i'll check over them next.

Status: Needs review » Needs work

The last submitted patch, 6: 2314289-6.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2314289-8.patch6.7 KB
new interdiff.txt2.57 KB

Fixed the tests, MigrateNodeTest needed to set d6_node_settings migration as complete since it's a new dependency.

MigrateDependenciesTest was using the comment migration to tests dependencies and comment depended on node which now depends on node_settings.

benjy’s picture

benjy commented
StatusFileSize
new 2314289-9.patch9.48 KB
new interdiff.txt3.72 KB

I've changed the migrations to required where applicable. The only strange one is d6_menu been required for d6_block, seems wrong? But see what the bot says.

Status: Needs review » Needs work

The last submitted patch, 9: 2314289-9.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2314289-11.patch9.64 KB
new interdiff.txt1.32 KB

Fixed dependencies.

Status: Needs review » Needs work

The last submitted patch, 11: 2314289-11.patch, failed testing.

chx’s picture

chx commented

core/modules/migrate/config/schema/migrate.schema.yml should be:

migrate.migration.*:
  type: config_entity
chx’s picture

chx commented
Title: Track result of migrations » Track result of migrations and use it to properly define migrate_dependencies
benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2314289-15.patch10.09 KB
new interdiff.txt457 bytes

Done.

The docs aren't entirely clear on what else type: config_entity gives us but I think it's just more flexibility from the ridged mapping structure. https://www.drupal.org/node/1905070

chx’s picture

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

Check core/config/schema/core.data_types.schema.yml for what type: config_entity gives us -- not much. This is a great day when so much badly marked optional migrational die and we don't need to move anything else there any more. Thanks!

alexpott’s picture

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

Committed 453a445 and pushed to 8.0.x. Thanks!

+++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
@@ -116,6 +116,11 @@ protected function prepareIdMappings(array $id_mappings) {
+      // which will make the position of this more appropriate.
+      $migration->setMigrationResult(MigrationInterface::RESULT_COMPLETED);

It would nice to have a comment here as to why we're marking migrations complete. Looking at the code it seems we are ensuring all the dependencies of the migration under test are met. Perhaps https://drupal.org/node/2315489 can address this because I had to think to work this out.

  • alexpott committed 453a445 on 8.0.x 
    Issue #2314289 by benjy: Track result of migrations and use it to...

Status: Fixed » Closed (fixed)

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