Migrate in update mode not updating existing file but creating new file entry

Sorry I might be understanding the report incorrectly, but have you tried the "reuse" option? That will reuse the file if it exists already in the destination location, as well file_entity if there is one.

Or is it something else?

I'll throw in another vote for DavidPetit's use-case. This is how we'd like for it to behave.

Let's try this patch on for size. This adds a new boolean config option called update_on_reuse. If set to TRUE (and reuse is ALSO set to TRUE), then the file is re-downloaded but a new file entity is not created. If reuse is not TRUE then the option will have no effect. It also ensures image derivatives are cleared when the file is updated.

As stated in the issue summary, the current behavior of the module (without my patch) leads to what I believe is an invalid state: multiple files with the same exact URI. In my case that triggered a bug due to the way the animated_gif module uses the file APIs: #3250924: PSA: if you have duplicate file URIs you will see performance and memory problems. But any code attempting to load files by URI has the opportunity to be at least non-deterministic (who knows which file you'd actually get), plus the potential of being a memory hog.

My patch doesn't exactly fix the bug since it doesn't currently change the way files are imported until and unless my new "update_on_reuse" setting is used because I didn't want to change the behavior of the module out from under someone, but I don't really think there's a valid use-case for how the module currently behaves, and I think we should consider changing it.

There are 4 possible options of what this module could do if the filename and uri already exists in file_managed table:

1. Reuse the existing file entity and update the existing file on disk.
2. Rename the file to avoid a collision and create a new file entity.
3. Reuse the existing file entity and DO NOT update the existing file on disk.
4. Current default (and invalid) behavior of the module: Create a new file entity with the same exact URI as the old one.

I'm arguing we need to prevent option 4 from happening. I'd suggest that option 3 is of debatable merit and we should at least think about getting rid of it, but I can imagine a system where files never get updated so maybe it's worth the extra efficiency compared to option 1. Option 2 (renaming the file to avoid a collision) is probably the safest default, IMO. Once we get agreement about the desired behavior and options we can discuss how best to expose these via configuration while best retaining backwards compatibility (I don't think the current set of options makes a lot of sense, especially since some of them are mutually exclusive).

Beyond that, it would be nice if we could clean up existing duplicate files, but I don't know if there's a reasonable approach here. It really depends on how the files were used. If the same file is represented by several different file IDs it's plausible that those various file IDs are in use/referenced throughout the system. If they're just in fields that's easier, but if they're referenced in WYSIWYGs or in other less-accessible data structures provided by other modules we're never going to be able to find and fix all the references. Perhaps the best we can do is provide some sample code that cleans up obvious dupes that people can use as a starting point to customize for their own purposes.

Hello @azinck! Thanks for taking the time to think about this one and for submitting a patch!

A few thoughts/comments:

• You are indeed correct that current default behaviour of this module is wrong! The behaviour is actually supposed to be to just replace existing files with the same URI. The fact that we're creating a new file entity is just a bug.
• I don't think we need a separate option for this since it's supposed to be the default. I can't imagine that someone is relying on the previous behaviour so I don't really think fixing it introduces a breaking change.
• To your comment about the reuse option being "of debatable merit and we should at least think about getting rid of it": I actually specifically built this module with that need. The use-case was that the images were on a nightly import with a bunch of other entity data and if the images changed at the source, the filenames would change. So if the filenames were the same we didn't need to pull them since we knew they were unchanged. But also, the "reuse" option is a feature of the FileCopy plugin (and of the core \Drupal\Core\File\File\FileSystemInterface::copy() method which FileCopy uses) so we're just keeping existing functionality intact

As it turns out, I actually need to update the module right now for D9 anyway. Our FileImport plugin extends the core FileCopy migrate plugin and the options for the existing file behaviour is inherited from it. Since #2877839: Reuse option in FileCopy migrate process plugin not work with remote files landed, the option has actually changed from the separate replace/rename/reuse options to a single file_exists option. The old keys were deprecated in D8 and have actually been removed in D9 so they won't even work right now on a D9 site. During that update, I'll be taking care of this bug. Will post back when it's fixed!

Thanks for the response, drclaw.

Yeah, I see the mismatch with the file_exists option now on the FileCopy plugin. I'm actually getting away with this being ok for my use-case because the default value (replace) matches my desired behavior, so I didn't catch that things wouldn't work properly if you wanted to do anything else.

So, FWIW, I'm using this with D9 but it's only working because my needs fit. So anyone else trying to use my patch as it sits right now should keep that in mind. That said...if you didn't want the "replace" behavior you probably wouldn't be searching out this issue, so I suppose it's probably still useful-enough for folks like me.

Since you're indicating you're doing a more significant rewrite I'll hold off on fixing things more fully. Thanks for your work!

Hey @aznick just reporting back that the fix for this issue has been pushed (as well as the configuration option update for existing file handling that I was mentioning). There's a new 2.1.0 release that includes both these updates. I increased the minor version since the existing file handling stuff makes this module no longer compatible with Drupal 8.

Technically I did push a fix just for this issue to the 2.0.x branch for anyone that might still be on D8, but if you're on D9 you should definitely switch over to the 2.1.x branch releases.

Thanks!
drclaw