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 d6_file that suggestions that you might really, really want null out the fid in d6_file so it doesn't cause issues with d6_user_picture_file. This is because user photos in d6 are not managed files. Since they aren't, and if you re-run a migration to add new node content that have managed fids. Those fids are going to start writing over the user photo ids added by d6_user_picture_file.

Proposed resolution

Add a comment to d6_file.

Remaining tasks

Create a patch to do this.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#5 interdiff-2605664-2-5.txt1.8 KByogeshmpawar
#5 add_comments_to_d6_file-2818155-5.patch1.35 KByogeshmpawar
#2 add_comments_to_d6_file-2818155-2.patch1.35 KBedgewl2

Comments

heddn created an issue. See original summary.

edgewl2’s picture

edgewl2 commented
Status: Active » Needs review
StatusFileSize
new add_comments_to_d6_file-2818155-2.patch1.35 KB

This patch adds the necessary comments

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Issue summary: View changes

Updated IS

mikeryan’s picture

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

I'd like to see more precise wording.

  1. +++ b/core/modules/file/migration_templates/d6_file.yml
@@ -13,6 +13,9 @@ source:
+    # If you are using this file and d6_user_picture_file to build a custom
+    # migration consider removing the fid fields to allow incremental migrations,
+    # because d6_user_picture_file in drupal 6 isn't management file.

    Suggested text:

    If you are using both this migration and d6_user_picture_file in a custom migration and executing migrations incrementally, it is recommended that you remove the fid mapping here to avoid potential ID conflicts. 

  2. +++ b/core/modules/user/migration_templates/d6_user_picture_file.yml
@@ -11,6 +11,9 @@ source:
+    # If you are using this file and d6_user_picture_file to build a custom
+    # migration consider removing the fid fields to allow incremental migrations,
+    # because d6_user_picture_file in drupal 6 isn't management file.

    Suggested text:

    If you are using both this migration and d6_file in a custom migration and executing migrations incrementally, it is recommended that you remove the fid mapping from d6_file to avoid potential ID conflicts.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new add_comments_to_d6_file-2818155-5.patch1.35 KB
new interdiff-2605664-2-5.txt1.8 KB

Updated patch as per comment #4, also added interdiff.

mikeryan’s picture

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

Looks good, thanks!

catch’s picture

catch
he/him
Primary language English
commented

Per discussion in #2748609: [meta] Preserving auto-increment IDs on migration is fragile should we open an issue for d6_user_picture_file to 1. always run after d6_file if it doesn't already 2. bump the auto-increment ID before it runs?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

re #7:
1) this is already done.

migration_dependencies:
  # Every migration that references a file by Drupal 6 fid should specify d6_file as an
  # optional dependency.
  optional:
    - d6_file

2) I think this is part of the longer term solution. This is a quick-fix while we discuss better solutions. We discussed the general problem in #2748609: [meta] Preserving auto-increment IDs on migration is fragile at a recent migrate weekly meeting and this was the stop-gap issue that was opened while we discussed long-term fixes.

catch’s picture

catch
he/him
Primary language English
commented

Right but I think bumping the auto-increment ID before the user picture starts is a different fix to bumping it after other ID-to-ID migrations run - it'd be specific to this one.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

I follow the logic about this being different. When I opened things, I opened two different issues. One for general entity_ids, and one for this specific issue with D6 user profile photos. See #2818157: Add comments to remove entity ids in migration for the other issue.

I don't think we disagree. Unless you think this issue here should do the bumping. I'd prefer to push the bumping ids off to a follow-up.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

I've added some thoughts on how exactly auto-increment fudging would work at #2748609: [meta] Preserving auto-increment IDs on migration is fragile. I don't think our future plans there really affect the advice we're giving here and how.

catch’s picture

catch
he/him
Primary language English
commented

We should add a specific child issue of that one for the 6.x user picture fix - it's not going to be handled by incremental migrations in general.

  • catch committed dbf488c on 8.3.x 
    Issue #2818155 by Yogesh Pawar, edgewl2: Add comments to d6_file stating...

  • catch committed 9326d5e on 8.2.x 
    Issue #2818155 by Yogesh Pawar, edgewl2: Add comments to d6_file stating...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Opened #2826047: 6-8 user picture migration must create new fids, which can conflict with fids used by other migrations because I think this specific issue needs tracking separately from the general one.

Status: Fixed » Closed (fixed)

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