Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Just discovered this code of MigrateDestinationFile::import()
while I was wondering why the updated fields of a field entity weren't stored:
if ($existing_files = file_load_multiple(array(), array('uri' => $file->uri))) {
// Existing record exists. Reuse it.
$file = reset($existing_files);
// TODO: Do we really need to save again? Copied from File Field.
// $file = file_save($file);
}
else {
// Get this orphaned file into the file table.
$file->fid = NULL;
$file->status |= FILE_STATUS_PERMANENT; // Save a write in file_field_presave().
$file = file_save($file);
}
Since files are entities too we really should consider to call the save. Otherwise the contents of the fields won't be updated even if the image is still the same.
Suggestion from my side - change the code to this:
if (!file_load_multiple(array(), array('uri' => $file->uri))) {
// Get this orphaned file into the file table.
$file->fid = NULL;
$file->status |= FILE_STATUS_PERMANENT; // Save a write in file_field_presave().
}
$file = file_save($file);
Comment | File | Size | Author |
---|---|---|---|
#7 | migrate.file_save.7-x-2-x.1314748-7.patch | 2.97 KB | donquixote |
Comments
Comment #1
mikeryanSeems to make sense - Moshe, you committed the lines in question, any thoughts?
Thanks.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedCommitted suggested fix to 7.x-2.x. Looks right to me. I think I was trying to save cycles but I miunderstood that file_save() is not part of file api but rather entity api.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedComment #4
das-peter CreditAttribution: das-peter commentedThank you very much for the change - unfortunately it looks like I missed one important thing before.
I played around with the change today and got nice exceptions like this one (I wonder why I didn't get them before):
I don't get the point why this happens - this is a freak-in UPDATE statement and not an INSERT. But it's likely I don't know enough about MySQL to understand that or my setup is somehow broken (I've the Media Module with its File Entity Module installed).
Thus it would be nice if someone could give this a try - if the error happens on other setups I suggest to change the code to remove the
$file->uri
before update:Better ideas very much appreciated :D
Comment #5
drewish CreditAttribution: drewish commentedI'm guessing you're not running into a Migrate bug. The URI field has a unique key but it's not case sensitive so if you have "public://foo" and "public://Foo" that'll throw the exception. See #966210: DB Case Sensitivity: system_update_7061() fails on inserting files with same name but different case for more.
Comment #6
donquixote CreditAttribution: donquixote commentedSomething else is wrong with the code in MigrateDestinationFile::import() for file saving.
$file->uri is NOT the primary key for files!
The primary key is $file->fid.
Effect:
The migration will create new files, instead of updating existing ones.
In some cases this might not matter, but it does matter if you create stub files (such as, for user avatars).
The following things have to be checked:
- Is the $file->fid set, or are we creating a new file?
- If $file->fid is set, does this file actually exist?
- Does another file exist with the same filepath (uri)? Is this our friend that has the same fid, or is this something else?
- If $file->fid is set, and the file entity with that fid does exist already, do we have a modified uri?
So, here is the logic we should be using:
Or am I missing something?
Comment #7
donquixote CreditAttribution: donquixote commentedAmazing, now this does actually work :)
Here is a very basic patch.
EDIT:
There is probably a lot of redundancy with other parts of code, where things are already being checked.
Also, the dedupe stuff that I talk about in the comments, needs to happen somewhere. This all depends how the $file objects are prefilled with uri etc, stuff that is happening before this function.
Honestly, I did not study any of this.
Comment #8
donquixote CreditAttribution: donquixote commentedBtw, about dedupe, there is also this beast still lurking.
#1358318: dedupe() overdedupes on --update
(for anyone who tries to work on this, this will be useful to know)
Comment #9
mikeryanAll this stuff about URIs has nothing to do with the original issue, it should be opened as a separate issue.
Comment #10
donquixote CreditAttribution: donquixote commentedDamn, now I don't remember any of this.
Issue: #1400284: Offer options on how to handle uri collisions
I could wait for a better time to post this, but there is also the chance that I will totally drop it. So I rather do it now.