Follow-up to #2748609: [meta] Preserving auto-increment IDs on migration is fragile

Problem/Motivation

Due to #2748609: [meta] Preserving auto-increment IDs on migration is fragile, we can get a quick win by just adding a comment into node, taxonomy, user and d7_file migration templates that suggestions that you might really, really want null out the entity id so you can actually possibly do incremental migrations.

Proposed resolution

Add a comment to all these migration templates saying you might want to remove the entity id.

Remaining tasks

Create a patch to do this.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#22 interdiff-18-22.txt1.03 KBada hernandez
#22 add_comments_to_remove-2818157-22.patch6.51 KBada hernandez
#13 add_comments_to_remove-2818157-13.patch5.45 KBada hernandez
#13 interdiff-10-13.txt1.68 KBada hernandez
#10 interdiff-7-10.txt4.54 KBada hernandez
#10 add_comments_to_remove-2818157-10.patch7.13 KBada hernandez
#8 interdiff-3-7.txt8.41 KBada hernandez
#8 add_comments_to_remove-2818157-7.patch8.3 KBada hernandez
#5 interdiff-3-5.txt8.41 KBada hernandez
#5 add_comments_to_remove-2818157-5.patch8.41 KBada hernandez
#3 add_comments_to_remove-2818157-3.patch9.46 KBada hernandez
#18 interdiff-13-18.txt2.29 KBada hernandez
#18 add_comments_to_remove-2818157-18.patch5.48 KBada hernandez

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Issue summary: View changes
ada hernandez’s picture

ada hernandez commented
Status: Active » Needs review
StatusFileSize
new add_comments_to_remove-2818157-3.patch9.46 KB

I have added a patch with the commentaries in migration templates.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/file/migration_templates/d7_file.yml
@@ -13,6 +13,8 @@ source:
+  # the fid and uid fields to allow incremental migrations.

    The comment for UID should be closer to the uid field. Can we move the uid field up higher so it is right next to the fid? 

  2. +++ b/core/modules/node/migration_templates/d6_node.yml
@@ -9,6 +9,8 @@ process:
+  # the nid, uid and vid fields to allow incremental migrations.

    Same applies here for uid. Can we move it higher so the comment applies more closely? 

  3. +++ b/core/modules/node/migration_templates/d6_node_revision.yml
@@ -6,6 +6,8 @@ deriver: Drupal\node\Plugin\migrate\D6NodeDeriver
+  # the nid, uid and vid fields to allow incremental migrations.

    And here. Move uid closer. 

  4. +++ b/core/modules/node/migration_templates/d6_node_translation.yml
@@ -7,6 +7,8 @@ source:
+  # the nid and uid fields to allow incremental migrations.

    And here. 

  5. +++ b/core/modules/node/migration_templates/d7_node.yml
@@ -6,6 +6,8 @@ deriver: Drupal\node\Plugin\migrate\D7NodeDeriver
+  # the nid, uid and vid fields to allow incremental migrations.

    And here. 

  6. +++ b/core/modules/taxonomy/migration_templates/d7_taxonomy_vocabulary.yml
@@ -5,6 +5,8 @@ migration_tags:
   vid: machine_name

    I don't think it makes sense to remove this one. The vid isn't a numeric identifier for taxonomy vocabularies. 

  7. +++ b/core/modules/user/migration_templates/d6_user_role.yml
@@ -5,6 +5,8 @@ migration_tags:
+  # If you are using this file to build a custom migration consider removing
+  # the id field to allow incremental migrations.

    I think this is similar to vocabularies. We don't want to remove it here. 

  8. +++ b/core/modules/user/migration_templates/d7_user_role.yml
@@ -5,6 +5,8 @@ migration_tags:
   plugin: d7_user_role
 process:
+  # If you are using this file to build a custom migration consider removing
+  # the id field to allow incremental migrations.
   id:
     -
       plugin: machine_name

    I think this is similar to vocabularies. We don't want to remove it here.

ada hernandez’s picture

ada hernandez commented
StatusFileSize
new add_comments_to_remove-2818157-5.patch8.41 KB
new interdiff-3-5.txt8.41 KB

@heddn thanks for your feedback, I have removed the commentaries unnecessary.

ada hernandez’s picture

ada hernandez commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: add_comments_to_remove-2818157-5.patch, failed testing.

ada hernandez’s picture

ada hernandez commented
Status: Needs work » Needs review
StatusFileSize
new add_comments_to_remove-2818157-7.patch8.3 KB
new interdiff-3-7.txt8.41 KB

@heddn now is the correct patch :).

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/file/migration_templates/d7_file.yml
@@ -13,7 +13,10 @@ source:
+  # the fid and uid fields to allow incremental migrations.

    The uid mapping should not be removed. If IDs are not preserved, the uid mapping should use the migration plugin instead of a straight mapping - but we have another issue to do that in general. 

  2. +++ b/core/modules/node/migration_templates/d6_node.yml
@@ -9,14 +9,16 @@ process:
+  # the nid and uid fields to allow incremental migrations.

    Ditto, uid mapping should not be removed. 

  3. +++ b/core/modules/node/migration_templates/d6_node_revision.yml
@@ -6,14 +6,16 @@ deriver: Drupal\node\Plugin\migrate\D6NodeDeriver
+  uid: node_uid

    Ditto. 

  4. +++ b/core/modules/node/migration_templates/d6_node_translation.yml
@@ -7,14 +7,16 @@ source:
+  # the nid and uid fields to allow incremental migrations.

    Ditto. 

  5. +++ b/core/modules/node/migration_templates/d7_node.yml
@@ -6,14 +6,16 @@ deriver: Drupal\node\Plugin\migrate\D7NodeDeriver
+  # the nid and uid fields to allow incremental migrations.

    Ditto, and etc. for the rest (I need to leave atm, haven't reviewed the rest...).

ada hernandez’s picture

ada hernandez commented
Status: Needs work » Needs review
StatusFileSize
new add_comments_to_remove-2818157-10.patch7.13 KB
new interdiff-7-10.txt4.54 KB

@mikeryan thanks. I have removed the uid for templates migration now.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work

So, to be clearer and more complete than my dashed-off review yesterday - we only need to remove ID mappings from a migration when they are the primary ID of the entity being created by that migration. There are a number of subsidiary migrations which do not create entities themselves, where the primary ID is of an existing entity being updated - that ID must be mapped for the right entity to be updated.

With the changes below, I'll be ready to RTBC this. Thanks!

  1. +++ b/core/modules/taxonomy/migration_templates/d6_term_node.yml
@@ -6,6 +6,8 @@ deriver: Drupal\taxonomy\Plugin\migrate\D6TermNodeDeriver
+  # If you are using this file to build a custom migration consider removing
+  # the nid field to allow incremental migrations.

    Actually, this one should not be removed, because it is properly mapping to the migrated nid (whether nids were preserved or not). 

  2. +++ b/core/modules/user/migration_templates/d6_profile_values.yml
@@ -10,6 +10,8 @@ source:
+  # If you are using this file to build a custom migration consider removing
+  # the uid field to allow incremental migrations.

    Similar to my previous uid reference - this must be mapped, and really should be using the migration plugin to work when IDs are not preserved, but we have a separate issue for that. 

  3. +++ b/core/modules/user/migration_templates/d6_user_contact_settings.yml
@@ -8,6 +8,8 @@ source:
+  # If you are using this file to build a custom migration consider removing
+  # the uid field to allow incremental migrations.

    Ditto.

ada hernandez’s picture

ada hernandez commented

@mikeryan thank you very much, I'll work in that

ada hernandez’s picture

ada hernandez commented
Status: Needs work » Needs review
StatusFileSize
new add_comments_to_remove-2818157-13.patch5.45 KB
new interdiff-10-13.txt1.68 KB

I updated the patch I removed the commentary for the ID must be mapped now I think this is correct.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Reviewed & tested by the community

That does it, thanks!

ada hernandez’s picture

ada hernandez commented

It is a pleasure to help community

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/node/migration_templates/d6_node.yml
@@ -9,6 +9,8 @@ process:
+  # If you are using this file to build a custom migration consider removing
+  # the nid field to allow incremental migrations.
   nid: tnid
   vid: vid

+++ b/core/modules/node/migration_templates/d6_node_revision.yml
@@ -6,6 +6,8 @@ deriver: Drupal\node\Plugin\migrate\D6NodeDeriver
+  # If you are using this file to build a custom migration consider removing
+  # the nid field to allow incremental migrations.
   nid: nid
   vid: vid

+++ b/core/modules/node/migration_templates/d7_node.yml
@@ -6,6 +6,8 @@ deriver: Drupal\node\Plugin\migrate\D7NodeDeriver
+  # If you are using this file to build a custom migration consider removing
+  # the nid field to allow incremental migrations.
   nid: nid
   vid: vid

+++ b/core/modules/node/migration_templates/d7_node_revision.yml
@@ -6,6 +6,8 @@ deriver: Drupal\node\Plugin\migrate\D7NodeDeriver
+  # If you are using this file to build a custom migration consider removing
+  # out the nid field to allow incremental migrations.
   nid: nid
   vid: vid

Wouldn't you also have to comment the vid for the node migrations? (I might be wrong - but I think I had to do this on a recent migration).

Also aren't we missing comments?

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work

Ah, right, should have caught that, thanks!

ada hernandez’s picture

ada hernandez commented
StatusFileSize
new add_comments_to_remove-2818157-18.patch5.48 KB
new interdiff-13-18.txt2.29 KB

Hi everyone, it did the changes for node templates

ada hernandez’s picture

ada hernandez commented
Status: Needs work » Needs review
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work

There's also alexpott's second comment:

Also aren't we missing comments?

I.e., we need to also document this for the cid mappings in d6_comment and d7_comment.

ada hernandez’s picture

ada hernandez commented

Oh!, ok thanks

ada hernandez’s picture

ada hernandez commented
Status: Needs work » Needs review
StatusFileSize
new add_comments_to_remove-2818157-22.patch6.51 KB
new interdiff-18-22.txt1.03 KB

I have added commentaries for comment migration template

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

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

Committed and pushed f1b66f3 to 8.3.x and d76893e to 8.2.x. Thanks!

Adding to 8.2.x because this is just documentation.

  • alexpott committed f1b66f3 on 8.3.x 
    Issue #2818157 by Adita, mikeryan, heddn, alexpott: Add comments to...

  • alexpott committed d76893e on 8.2.x 
    Issue #2818157 by Adita, mikeryan, heddn, alexpott: Add comments to...

Status: Fixed » Closed (fixed)

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