Still working out what's happening here but it seems that if a node migrations failed because of something like an integrity constraint failure(node title being empty of some bug in a migration) then the term_node migration will try to modify the node that doesn't exist and write it. Which will fail with an integrity constraint on the empty title.

Testing a patch to confirm this to be the case.

CommentFileSizeAuthor
#5 2588945-5.patch4.67 KBphenaproxima
#3 2588945-3.patch3.47 KBphenaproxima
#2 2588945-2.patch1.25 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2588945-2.patch1.25 KB

Added a bit of defensiveness to the d6_term_node and d6_term_node_revision migrations. Still needs tests, but I expect this will help.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: -Needs tests
FileSize
2588945-3.patch3.47 KB

I worked very, very hard and I think I deserve a test.

A test?

A test.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented 
+++ b/core/modules/taxonomy/src/Tests/Migrate/d6/MigrateTermNodeTest.php
@@ -51,4 +55,28 @@ public function testTermNode() {
+    $node = $this->sourceDatabase->select('node', 'n')
+      ->fields('n')
+      ->condition('nid', 2)
+      ->execute()
+      ->fetchAssoc();
+    $row = new Row($node, $migration->getSourcePlugin()->getIds());

$node does not need to be fully populated, ['nid' => 2] will do.

Other than that, looks good for what it does. I'll repeat what I've said previously, though - we should not have separate term_node* migrations, term references should be migrated as part of the node migration just like any other field. This bug fix holds us over until we address that (I'll open an issue).

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
FileSize
2588945-5.patch4.67 KB

Moved the gnarly set-up stuff in testSkipNonExistentNode() into a helper method of MigrateTestBase. I think this is a scenario we may want to test again, so it makes sense to have a helper for that. Also addressed @mikeryan's comment above.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#2597650: Term references should be incorporated into D6 node migration

RTBC pending successful automated tests.

Added issue for merging term_node into node migrations: #2597650: Term references should be incorporated into D6 node migration.

webchick’s picture

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

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 4e760c3 on 8.0.x 
    Issue #2588945 by phenaproxima, mikeryan, neclimdul: term_node migration...

Status: Fixed » Closed (fixed)

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