While working on a D6 -> D8 migration, I noticed that over 90% of the items for the d6_term_node_revision migration were being ignored, which seemed high. Using xdebug to trace the migration, I identified some specific items that were being ignored that I was pretty sure should be valid. I believe the issue is with the process specification in d6_term_node_revision.yml:
process:
vid:
-
plugin: migration_lookup
migration: d6_node
source: vid
-
plugin: skip_on_empty
method: row
It's trying to translate the node revision ID (which doesn't actually need translating (but might in the future)) as a means of ensuring that items for which the node doesn't exist are skipped (https://www.drupal.org/project/drupal/issues/2588945). However, it's specifying d6_node, meaning it looks in migrate_map_d6_node_[content_type], which maps node IDs, not revision IDs. Hence why about 6% of the items, in my case, are getting through - those are the ones that are lucky and happen to have a revision ID that's also a valid node ID.
I thought I could just change the migration parameter for migration_lookup in the above code to d6_node_revision, but that results in all items being ignored (because the migration plugin manager is finding no instances for that migration ID in migration lookup's transform function), so I seem to still be missing a piece of the puzzle.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 2934799-20.patch | 3.73 KB | maxocub |
| #20 | 2934799-20-test-only.patch | 3.25 KB | maxocub |
| #20 | interdiff-2934799-17-20.txt | 1.07 KB | maxocub |
Comments
Comment #2
olarin commentedAha! Changing it to look to d6_node_revision for the migration_lookup does indeed work; there's just a corresponding change that also needs to be made in the Migrate Upgrade module, in case you're using the contrib modules to try all this from the command line via drush (which of course you are because how else can you reasonably try migrations one at a time), to make sure it remembers to have the derivative d6_node_revision migration IDs on hand to sift through.
Here's a patch for this issue. Will be submitting corresponding patch to Migrate Upgrade momentarily.
Comment #3
olarin commentedComment #4
olarin commentedCorresponding patch to Migrate Upgrade: https://www.drupal.org/project/migrate_upgrade/issues/2935225
Comment #5
olarin commentedPatch for 8.4.x. Actually differs slightly because the file location has changed between versions (directory was renamed).
Comment #7
olarin commentedA patch for 8.6.x, identical to the patch for 8.5.x, just proving it still applies.
Comment #8
heddnAssigning to myself to review. This does seem like a major issue. So leaving the priority.
Comment #9
dwwAgreed this is a major (if not critical) bug in the D6 term migration. I just saw this last night on a D6 migration I'm working on. Seeing this issue totally explains what happened. Here's the output of trying to run my term_node and term_node_revision migrations:
The term_node output is all as expected, but the numbers are totally off from reality for term_node_revision*.
I'll be re-running the migration this afternoon with the equivalent fixes from @Olarin to both the migration .yml files and migrate_upgrade. Stay tuned for the results, but I suspect they'll be much more promising.
Thanks!
-Derek
Comment #10
heddnThis is a pretty obvious break/fix. However, we should probably add a test for this.
Comment #11
heddnComment #12
heddnI'm the one who originally put this back to NW. This is obviously a broken thing. Let's just fix it. I am not sure there is much value in a test.
Comment #13
dwwIn general, +1 to the RTBC, although the migration where I saw this ended up not caring about migrating revisions. But, I did try it once and it worked. It's such an obvious fix.
Thanks,
-Derek
Comment #14
alexpottI think we should have some test coverage of this. Since if we reverted the fix no test would fail.
Comment #15
olarin commentedI haven't had time to circle back to this and write any tests yet. While I'm a proponent of maintaining good test coverage, I wonder if, for a bug this egregious and a fix this obvious, it wouldn't be acceptable to go ahead and commit the fix and open up a followup issue for adding test coverage if deemed necessary. Of course there's a substantial risk that the followup issue will fall to the back of the queue and get ignored, but that still seems better to me than the fix not getting committed for months on end, given that right now node revision migrations are almost completely broken and have been for a long time.
Comment #17
maxocub commentedI investigated this and I found that the current tests pass because the revision ID used in the
migration_lookupis never absent from thed6_nodemapping table so no rows are ever skipped.One way to make a failing patch can be to change this low revision ID to one high enough so that it's not found in the
d6_nodemapping table and thus the row will be skipped.But with the fix applies, this new high revision ID will be found in the
d6_node_revisionmapping table and thus the row will be migrated.The test only-patch is the interdiff.
There might be some other test failures due to the fact that I changed a revision ID. Let's see how it goes.
Comment #20
maxocub commentedFixed the failing test.
Comment #22
heddnAnd now, with the tests, we can go back to RTBC.
Comment #23
alexpottBackported to 8.6.x as this is a major bug fix that improves migrations.
Committed and pushed d4eb909c23 to 8.7.x and abd11edbd0 to 8.6.x. Thanks!