6-8 user picture migration must create new fids, which can conflict with fids used by other migrations

Set the autoincrement ID to a high number before the user picture migration runs, to give some space for other migrations to add existing IDs in the gap left.

In this instance, perhaps we could more simply set explicit fids as $max_existing_fid + SOME_FAIRLY_LARGE_CONSTANT?

Discussed in the migrate weekly meeting. The proposed solution:

• Need a failure test to demonstrate the issue now.
• Remove the fid from the d6_file template.
• Update the d6_file/d6_user_picture migration docs to denote the current state.

This should catch 98% of sites that don't really care what the FID is. In the rare condition that FIDs are important, then the site could easily turn on migration of the fids and figure out a solution for user pictures.

Assigning to myself to work on this.

Still needs tests. But this gets the ball rolling.

Here's some tests.

Discussed with @catch, @cilefen, @effulgentsia and @xjm. We agreed that this should be a critical issue since regardless of the experimental nature of migrate this is a data integrity issue that can result in data loss.

Assuming the migration is working as it should (i.e. each file is migrated twice due to having a different uri the second time the d6_file migration is executed) and assuming I understand what should be happening (!), then I have adjusted MigrateFileTest to pass correctly. I have added comments to try and explain what is happening and why.

Unassigning.

re #13 it looks like your patch and interdiff are the same thing. I'm pretty sure you intended something different.

Sorry, my git-fu was a little off yesterday. Here is what I meant to upload. :#

1. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
   @@ -90,12 +90,15 @@ public function testFiles() {
   -    $file = File::load(2);
   +    // File 2, when migrated for the second time, is treated as a different file
   +    // (due to having a different uri this time) and is given fid 6.
   +    $file = File::load(6);
        $this->assertIdentical('public://core/modules/simpletest/files/image-2.jpg', $file->getFileUri());
   

   I think we should also test the map here. To should the before 2 maps to 2 and now 2 maps to 6.

2. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/MigrateFileTest.php
   @@ -90,12 +90,15 @@ public function testFiles() {
   +    // File 6, created in static::migrateDumpAlter(), shares a path with
   +    // file 4, which means it should be skipped entirely. If it was migrated
   +    // then it would have an fid of 9.
   +    $this->assertNull(File::load(9));
   

   This test looks brittle - perhaps it is best test would be to count the number of files before and after the second migration run. Not sure.

3. One thought is that if we do #2748609: [meta] Preserving auto-increment IDs on migration is fragile is this change and the complexity brings (of not preserving FIDs) really necessary?

If we fix the general problem, then we don't need one for these files. But I think we can get this in quick and dirty, while auto-increment could take longer.

1. Test the map (before and) after the second migration execution.
2. Test the number of files migrated.
3. I agree with @heddn that we should go for this as a quick and dirty solution.

Looks good with these last changes. Unfortunately, I cannot mark RTBC. Hopefully someone else will jump in.

Hi,

The patch #21 seems to be look good. Working as expected.

@arunkumark, please provide a more specific review than that when RTBCing a patch. "Working as expected" does not actually contain any information. Please document what you reviewed and how you tested it. Otherwise, you will not receive issue credit for your review. Thanks for helping review patches!

Looks good to me, thanks!

@alexpott points out my RTBC was even terser than @arunkumark's... I did test this locally and it worked as expected. I also reviewed the code in and of itself and in the context of previous replies here, and I feel the previous points have been addressed.