Changing node language by moving down language list deletes files in file field

Same problem for me, changing node language from english to language neutral delete file referenced by filefield...

Moving this to file module for now, I remember there being issues in file_field_insert()/update() before.

oh dear.

Fixing tag.

I confirm this bug

I can confirm this as well...

Does anybody knows how to fix it? That's a major bug! I think it should be fixed on D7 before even think to D8...

I made this change in file.field.inc in file_field_delete_file function:

function file_field_delete_file($item, $field, $entity_type, $id, $count = 1) {
  // To prevent the file field from deleting files it doesn't know about, check
  // the file reference count. Temporary files can be deleted because they
  // are not yet associated with any content at all.
  $file = (object) $item;
  $file_usage = file_usage_list($file);
  if ($file->status == 0 || !empty($file_usage['file'])) {
    file_usage_delete($file, 'file', $entity_type, $id, $count);
-    return file_delete($file);
+    //return file_delete($file);
  }

  // Even if the file is not deleted, return TRUE to indicate the file field
  // record can be removed from the field database tables.
  return TRUE;
}

As I use the media module, I can't see any situation where a file need to be deleted after a field change... hope i'm right.

No thats not ok :) you never going to delete files ever. :)

On the other hand this function has been changed in D8. So I wonder if it's still a bug there.

Actually, files are deleted because, as I said, I use the media module which handle files in a different way. You can remove a file from a field, but it still exist in the "media library", and if you want to delete it you have to go in this library and click on delete, thus media_multiple_delete_confirm_submit() is called, which calls file_delete().
Maybe that issue belong to the media module which has to alter file_field_delete_file to do it on its own way...
Back to D7 if the function has already been changed in D8.
Need confirmation for more experienced developers to move this issue to media module...

I just search for file_field_delete_file issues in media module and here's what I found:
#1239056: Remove Media at content edit page cause the file to be completely deleted from server/database?
And the linked core issue:
#1399846: Make unused file 'cleanup' configurable
So I mark this as a duplicate

This a problem with the media module NOT drupal core. It's caused by architectural decisions made in that module. So this can't be called a core bug. So yes this is a duplicate of the other issue.

Actually, I think sreynen is right. Although #1239056: Remove Media at content edit page cause the file to be completely deleted from server/database? should be corrected it belong to media module.
#13 is an easy and dirty workaround for those who use the media module, as it is critical for production sites, but for others, according to aspilicous in #14, files won't be deleted ever.
#2 is actually a start for a solution...

quick and ugly fix...
in file file_field.inc
added this:

$original_fids2 = array();
  if (!empty($entity->{$field['field_name']})) {
      foreach ($entity->{$field['field_name']} as $lang){
            foreach ($lang as $original_item) {
              $original_fids2[] = $original_item['fid'];
            }
          }
  }

before this

if (!empty($original->{$field['field_name']}[$langcode])) 

also changed the $current_items variable to $original_fids2
very ugly but works. can also be improved with a static variable for caching.

Here is a patch addressing this for D7, for which I need a (hopefully) proper fix.

The change in this patch is pretty invasive, but I think the current logic is deeply flawed and cannot be fixed without a radical change in the approach: currently every field and language is checked for changes in the referenced files in isolation, not taking into account that files might be moved around. Hence, as we are seeing here, removing and then re-adding back simply doesn't work, since meanwhile we lose the file on the file system.

The current patch implements a global check: all fields attached to the entity belonging to the correct field type (file o image, at least in core) are inspected, and all file occurences are collected both for the original values and the submitted values. The occurrences are then compared and only actually removed files are deleted from the file system. The check is performed in file_field_update() to minimize the impact of this patch and reduce the chances of BC issues. A static cache is used to avoid repeating it for each field of the same type and for the various field translations. The static cache is cleared right afterwards to avoid problems of stale data.

The check is extended to all fields of the same type because the issue here is not limited to the use case reported in the OP: if a file is referenced in one file field and then it's moved to another file field on the same entity, chances are that the it will be wiped exactly the same way reported in the OP.

I think this could be a reasonable approach to forward-port to D8, altough it does not address the (rare) case where a file is moved accross different field types. Probably a better way would be to simply drop automatical deletion of files that no longer have references, as discussed in http://groups.drupal.org/node/191183, but this would probably need some kind of file overview page as the one provided by Media.

Anyway, this patch should work in D7. Leaving the version to D7 so that the testbot will pick it up. I won't post a D8 forward-port nor test updates until we agree on a proper strategy to fix this.

Leaving to needs review so that we can get some feedback on the approach taken in #24.

Fixed an edge case.

see #1399846: Make unused file 'cleanup' configurable comment 8 for a testcase.
(advise there was... use file_lock)

Just a note: #26 is supposed to be good but it has been tested against the 8.x branch, AAMOF the error is it doesn't apply.

MMh, let's actually try and test #26 on the correct branch.

#26: drupal-files_wiped-1141912-26.D7.patch queued for re-testing.

Rerolled

Sorry, wrong merge.

Aw, windows newlines

Back to D8 to get feedback about #24.

#37: drupal-files_wiped-1141912-37.patch queued for re-testing.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Issue summary is being reviewed and updated by bmadore in DrupalCon Portland Core Mentoring sprint.

I am working on the reroll

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

I've reviewed the issue and edited the issue summary to update and conform to the correct format.

I've tried to replicate this issue in D8 doing the following:

• Created new D8 site using latest build
• Enabled Language, Content Translation and Interface Location modules
• Added two new languages via admin/config/regional/language
• Added file field to Article content type
• Configured file field for multiple file extensions
• Added a node of type Article uploading a text file using the file field
• Saved the node
• Clicked on the Edit tab
• Chose an alternate language from the Languages dropdown
• Saved the node

Following these steps, I did not encounter the issue described in this issue as occurring with the D7 Content Translation module. The file uploaded with the file field survived when I saved the node. I created several other nodes and tried several language changes. The files remain after editing.

This may not be a D8 issue after all. Can others confirm my findings?

I was able to reproduce the issue in D8.

1. Created new D8 site using latest build
2. Enabled Language, Content Translation and Interface Location modules
3. Added two new languages French and Spanish via admin/config/regional/language
   Will have three languages in the following Order:
   English
   French
   Spanish
4. Added file field to Article content type
5. Configured file field for multiple file extensions (txt, docx, pdf, doc, ppt, pptx, xlx, xls)
6. Created three nodes of type article one node on each language
7. Clicked on the Edit tab
8. Chose an alternate language from the Languages dropdown
9. Saved the node

The attached file is deleted in specific cases see #2
The attached is deleted depending on the position of the language.
If the language of the node is the last option in the language select and you change to the first option the attached file will be deleted.

I witnessed and confirmed ibakayoko's test at the PDX8 core sprint. But we both acknowledged that his build was a few days old. Given the numerous changes made even that day, I decided to re-test today (5/27) with a fresh install of Drupal 8.

I followed the same steps as ibakayoko in #46 and continued changing languages in nodes both up and down the list of 3 (and including the "not specified" and "not applicable" options). The file was not deleted no matter the original language or its position in the list. I even changed the order of languages and retested.

I suspect that changes have resolved the issue in D8. Can we confirm this? And if so, can someone identify the change that has solved the issue so we can backport the solution to D7?

can someone identify the change that has solved the issue so we can backport the solution to D7?

Try using git bisect.

D7 issue?

Is moving file deletes to cron a suitable solution for D7?

I don't think that can be backported without providing BC support, that is making the behavior optional. And if it is optional we need a different fix anyway.

@sreynen: "backward-compatibility support"

Yep, sorry :)

I think for D7 we still need to go with something like #37.

Updated issue summary.

Patch in #37 worked for me.

It was the same problem, but it appeared only when I edited the content setting the language from some specific to neutral one (which is above in the select options list) and saved it. Changing back from neutral to some specific language was OK. And from one specific to another was OK too.

Haven't reviewed the patch, but that looks like a testbot fluke so back to RTBC.

Back to RTBC

Actually this needs some more work, but I'll leave it to RTBC for now to get @David's feedback about the overall approach. Not worth spending more time on this until it's validated.

+++ b/modules/file/file.field.inc
@@ -255,40 +255,98 @@ function file_field_update($entity_type, $entity, $field, $instance, $langcode,
+  $processed[$entity_type][$id][$type] = TRUE;

We should add also the revision id, I think.

The basic approach looks reasonable to me. It would certainly be cleaner to do the whole thing in hook_field_attach_update() (and then I think supporting the case where the file moves from one field type to another would be simpler also) but I suppose you're right that for backwards compatibility reasons it's best to leave it where it is.

Back to needs work, I guess, for #62.