d6 to d8 migration throws integrity contraint warning with duplicate file paths

Marked #2504553: Ensure that Drupal 6 duplicate file names are correctly migrated as duplicate.

Looks great to me, but needs tests AFAICS.

This patch includes a test, as well as a completely refactored MigrateFileTest and a unit test of the file_uri process plugin.

Restored MigrateDumpAlterInterface to MigrateFileTest -- turns out it's important and prevents test failures due to concurrency.

This will need a reroll when #2534012: Move module-specific migration support into the file module lands.

FileUriTest should be moved into the File module's test suite and be in the file @group.

+++ b/core/modules/file/src/Plugin/migrate/process/d6/FileUri.php
@@ -24,20 +24,19 @@ class FileUri extends ProcessPluginBase {
+      return 'temporary://' . ltrim($uri, '/');
...
+    return ($is_public ? 'public' : 'private') . '://' . ltrim($uri, '/');

Can you explain the ltrim? Isn't the point that the base path has been removed which would have also included the slash?

+++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/d6/FileUriTest.php
@@ -0,0 +1,84 @@
+      '/path/to/private/files/baz.gif',
+      'sites/default/files',

Similarly, i find this interesting that the unit test isn't setting the temporary_path properties relative to the values being passed in?

Tried to reproduce the error without success. How can I do that? I created a duplicate file path and that was migrated to D8 just fine.

If no one can reproduce this within the next week or two, let's close it.

All righty, ready to go I think...

BULLCRAP. This is just PIFR being mean, and I've re-queued the patch for testing by DrupalCI to prove it. Restoring RTBC.

PostgreSQL is not liking this patch.

Ooooooooh! PostgreSQL, as it is wont to do, found a genuine (if edge-casey) problem. It's fixed now in the File source, but we'll need to address it in all sources.

Consider two file entities in the source database, with fids of 5 and 7, and imagine that they have the same timestamp. If the source plugin selects them and orders only by timestamp, they will appear in a non-deterministic order that can vary by database -- which is why PostgreSQL revealed this issue but MySQL and SQLite did not.

The solution is that sources which order on a high-water property need to also order on a unique field (fid, nid, uid, etc.) to force the order to be deterministic. This should be fixed for all sources, but let's do that in a follow-up issue. It will only be a problem when there's a high-water property collision, which is relatively unlikely anyway -- although the likelihood of it increases with the size of your data set.

Yay, lovin' all the new test coverage here, as well as postgreSQL is the culprit for fleshing out a real bug. ;)

Committed and pushed to 8.0.x. Thanks!

Growl.

Why does the order need to be deterministic? The only way I can see that is if we have two files with the same id and the same uri in the dumps? And that is surely invalid anyway?

d6 allowed conceptually invalid data

Can you expand on that?

So, if it's some niche little problem to files, it doesn't need applying to all sources as was said in #41?

I think the point is that the secondary sort isn't logically necessary - what does it matter whether it's deterministic or not? My suspicion is that the real problem is the tests are assuming a specific order, and the secondary sort is forcing the order to what the tests want? If so, I would suggest that it's the tests that need fixing.

Y'all have made me see sense with regard to making all sources order deterministically. @neclimdul is correct; the only reason it's important in this case is because the D6 files table is screwy. Paths/URIs are unique identifiers but D6 didn't treat them that way, thus the reason we need to do these acrobatics. But that's unique to files, and only D6 files at that -- virtually every other form of content we migrate from D6 has a single, unique identifier which renders order irrelevant.

So I was mistaken in saying that all sources should enforce a deterministic order. My brain must have been on a beach somewhere. I recant!