Regression: Files or images attached to certain core and non-core entities are lost when the entity is edited and saved

Same problem here.
If i add an image and save and then edit again, the image is there but if
I save again without editing anything, the image lost.

I confirmed the bug for image fields in Taxonomy terms. To test I added an instance of field_image, Article's default image field, to the Tags vocabulary. Images were lost on re-save.

I also added a text field to Tags. The example text was not lost on re-save. So it's possible this only impacts images.

I tested images in articles and added another instance of field_image to users. Neither of those fields lost their images on re-save of those entities.

Bumping to critical due to data loss.

Confirmed that this is a regression introduced in the Drupal 7.29 security release.

At first glance, it doesn't really look like anything that's the "fault" of the code introduced in the security release, but rather the same root cause as this preexisting bug: #1327224: Access denied to taxonomy term image. Code similar to what's found in comment #3 of that issue will fix the bug here, but doesn't look like the right fix because it will clobber whatever entity-level access control other modules (like the Entity module) apply to taxonomy terms.

I would also suspect, but haven't verified, that other (non-core) entities are affected as well... basically any entity-providing module which does not implement hook_file_download_access()?

I have a possible fix in mind, but it's scary... First I'm going to update the Drupal 7.29 release notes to point to this issue.

Here's the scary fix.

It seems to work, and it also appears to make the code in file_file_download() consistent with what the code comment higher up in the function says it's doing:

  // Loop through all references of this file. If a reference explicitly allows
  // access to the field to which this file belongs, no further checks are done
  // and download access is granted. If a reference denies access, eventually
  // existing additional references are checked. If all references were checked
  // and no reference denied access, access is granted as well. If at least one
  // reference denied access, access is denied.

So it makes sense to me at first glance. However, it's still very scary... :)

For some context, the $grants = array('system' => FALSE) line which I removed in the above patch was added here: #846296-43: file_file_download() only implements access checks for nodes and users

A slightly better patch, which preserves the ability of modules to return something like "0" from hook_file_download_access() and have it still treated as FALSE.

The same issue arises with custom field widgets defined through hook_field_widget_info(). The fix that I've discovered is to implement hook_file_download() and return the proper header if your custom URI should pass validation.

This may extend beyond the taxonomy module.

After updating to 7.29 I am experiencing the same issue, only with nodes instead of taxonomy and the imagefield_crop (I realize this isn't core, but thought it may help with debugging) contrib module.

1) Upload image
2) Save
3) View content to verify image is there
4) Edit content and change any other field (not the photo)
5) Save
6) Photo field is now empty.

The contrib module profile2 looks to be problematic as well.

I have the same problem using managed_file with form API in custom form.

The new call to file_download_access($file->uri) (line 515 of file.module) triggered this issue for me. When rolling file.module back to the 7.28 version, the issue goes away.

I'm aware that this isn't a hardy solution but hope it can help with debugging. :)

Patches #5 and #7 do not resolve the node and contrib instances of this issue.

Should we open a new bug for the non-taxonomy specific version of this bug?

I couldn't reproduce the issue with imagefield_crop. Maybe there are additional steps?

The only way I could reproduce this with nodes was to set up a (mostly artificial) situation where the user had access to edit the node but not view it. In that case, the previous patch doesn't help either.

Given that, and since the previous patch is also a larger change than would be ideal to fix this regression, here's another approach which is more of a workaround: The code comment that was right above the line of code changed by the security release says it only runs "if the FID has changed" but that's apparently not true (if it were true, we wouldn't have this bug). So we can make it behave more that way instead. Basically, if a file is already attached to the entity and no one is trying to change it, we can keep using the same file regardless of what file_download_access() returns. We should only ever need to ditch the file if it was changed and the user doesn't have access to attach the new one.

An attempt at implementing that is in the attached patch.

Basically, if a file is already attached to the entity and no one is trying to change it, we can keep using the same file regardless of what file_download_access() returns.

What if the current image is one that the user was never supposed to have access to in the first place?
Or what if the access rules change and the user is no longer allowed to access the image?

Basically, if a file is already attached to the entity and no one is trying to change it, we can keep using the same file regardless of what file_download_access() returns.

This would fix the urgent regression.

What if the current image is one that the user was never supposed to have access to in the first place?
Or what if the access rules change and the user is no longer allowed to access the image?

If someone can edit the image field then I think they need to be able to see the current image while editing, regardless of their access to the image itself -- that behavior should be the same in Drupal 7.29 (and before) as it is with this patch. If you can edit something, that implies a certain ability to see its existing value... For example, if you can edit a node but don't have access to view the body field, you still can see the body field on the edit form.

If you can't edit the image either (for example if https://www.drupal.org/project/field_permissions is used to control field-level editing access) then neither this patch nor Drupal 7.29 should affect anything, since the widget won't appear on the form at all. Although this scenario is worth double-checking for regressions.

Unfortunately, the test failure above looks real (related to default images). I did some manual testing of the patch with a field that had a default image before posting it and all looked OK, but apparently something is wrong.

I wrote a quick and dirty script to see if you site is affected, you can run it with drush scr: https://gist.github.com/scor/de1058e2d80f81205c04

For what it's worth, some co-workers and I were investigating a possible issue on a site we're working on where images might be "falling off" of Commerce product entities ("commerce_product") with an image/file field, but we aren't sure why. When we found this issue, we thought it might be the cause, but we still couldn't replicate it by following the instructions in the OP. So this problem does not seem to apply to all entities with file fields.

Regarding #18. Thanks scor!

In my own testing, I followed the if statement with an else clause:

else echo "nada\n";

I was new to using drush to run a php script, and found a thread here: http://drupal.stackexchange.com/questions/9837/how-to-execute-php-script...

Steps I took:

$ touch 2305017_file_lost.php
$ pico 2305017_file_lost.php
pasted the code from https://gist.github.com/scor/de1058e2d80f81205c04#file-2305017_file_lost..., and amended it as noted above.
$ drush @dev php-script 2305017_file_lost.php
where @dev is the drush alias for my development site.

P.S. My result was negative.

#13 fixed the issue for me for taxonomy terms. I wasn't seeing the issue with other non-node entity types (field collection).

I assume this is going to need a test...

What would prevent file_download_headers() (includes/file.inc line 2032) from returning an array of headers with a valid URI? This seems to be where the problematic contrib modules are failing after the 7.29 upgrade.

Do modules supplying fields need to implement 'hook_file_download' with the new call to file_download_access($file->uri) on line 515 of file.module now?

It looks like file_download_headers() must return at least one array item for this to function, even on public (non-private) image files.

@citricguy those are good questions that naturally require tests. see #22.

I've not created tests in this context before, but would love to be more involved with core so I'll take a crack at it this weekend.

I believe any module that uses the File API will now be required to implement hook_file_download() due to the changes to file.module in Drupal 7.29. Without this hook being implemented file_download_access($file->uri) will never get the array item it needs to return 'count > 0' which triggers an iteration of this bug.

Modules that did not need 'private' file storage previously could get by without hook_file_download() but this is no longer the case.

Example workaround/temp-fix for a contrib module...

/**
 * Implements hook_file_download().
 */
function hook_crop_file_download($uri) {
  return file_file_download($uri, 'FIELD_TYPE');
}

Here's a new version of #13 with some changes intended to make it pass existing tests.

@citricguy, it would be great if you have a chance to write tests for the regression and add them to the patch - thanks for offering to try!!

I think you may be right that this release also inadvertently introduced the requirement to implement hook_file_download()... (Contrib modules should have been doing that anyway to support private files, but custom code for an individual site may have had no need to.) I think the patch in this issue will fix that problem as well, and restores the situation such that there's no requirement to implement hook_file_download() that didn't exist in the past. Do you know of a good example to test manually with?

modifying title and description to reflect other reports that the issue extends beyond just the taxonomy entity

fixing typo in title

Confirming this is affecting "file_entity"s as well with the media module (https://www.drupal.org/project/file_entity). Was about to start debugging today before I received notification of the bug.

I'm going to look to see if I can write a test in the next hour.

It's not self-evident to me that the patch would fix the situation of a file being added, but the file API is a bit obtuse.

Here's a start to a new test - don't have time to go through the edit/save/check steps now.

I tried adding an image to an article as authenticated user from two different browsers this morning. (7.29 on 19 July)

It worked fine on preview, but when I tried to save the article (or remove the image), I got

Fatal error: Call to undefined function file_create_path() in /home/ghookerm/public_html/aimable/sites/all/modules/fbsmp/plugins/photo.inc on line 516

This was from an article, not a status. Adding images to a status seems to be working.

@oneblankspace your bug comes from the fbsmp module, it's therefore off topic here. Please file an issue against the fbsmp module instead: https://www.drupal.org/project/issues/fbsmp?categories=All.

I think file_download_access() should always return TRUE for "public" files. Modules define hook_file_download() with private files in mind. It is wrong to expect headers from hook_file_download() for a file under "public" file system.

For instance, file_download_access("public://foo.jpg") says that I don't have access to "public://foo.jpg" because there is no module returning headers for it. However, i can download it at http://example.com/sites/all/files/foo.jpg as expected, which proves that access check must be skipped for files that are served directly by the web server.

with #31 , I was able to add, edit and save an image. And still see the image

Here's a test only that has 1 fail basically following the original STR, plus a combined patch with David's fix from #26

Here's a slight addition to the test to also check the term loaded from the DB. This could suffice instead of the xpath check if that seems fragile.

My apologies for my partial pollution of this issue.
With regards to #26:

... this release also inadvertently introduced the requirement to implement hook_file_download()... (Contrib modules should have been doing that anyway to support private files, but custom code for an individual site may have had no need to.)

1. When should a contrib module implement hook_file_download()?
2. In order to support private files, is there a default/guideline/example implementation which would cover this requirement?

After having gone through this issue, related issues, and API docs, I am not clear on what is required for a contrib module to properly support private files. I think that clarifying this would help reduce the fallout of this regression, and future security issues related to private files.
Would this cover it?:

contrib_file_download($uri) {
  return NULL; // Assuming the related module does not do any access control itself.
}

I think #37 addressed everything so RTBC.

Looks like we're close but can we get a bit more manual testing of this, to make sure it fixes the problem in the various reported scenarios (and to make sure it doesn't cause regressions with any complicated cases, like Media or File Entity)? Thanks!

We should be able to commit this as a hotfix and release Drupal 7.30 very soon, though... early this week hopefully.

In the meantime, the test looks good to me but the "edit terms in 1" permission isn't robust cross-platform, and it shouldn't be needed anyway if the test user has "administer taxonomy". This version removes that, and also cleans up a couple comments, etc. The functional code has not changed since #26, so any manual testing people have done since then is still valid.

@Phizes, if a contrib module is providing a new field type that "extends" the standard file field in some way (like Image module does) then it should probably do what Image module does => basically call file_file_download($uri, 'your_field_type_goes_here') so as to inherit the same access controls as file fields. If it is doing something more complicated with managing uploaded files, it may need a more complicated implementation.

I think this only affects a small number of modules though.

#42 worked for me, thanks. (my case: images attached to terms)

I've the same problem with the background supersized images of any page they are.
I'm trying to solve with #42 patch but I don't be able to do this simple operation. It's the first time I'm trying to apply a patch to a module core file. I'm a mac user, anyone can help me?

@nicotine

You need to have a git checkout of Drupal 7.x on which you apply the patch.

https://www.drupal.org/node/3060/git-instructions/7.x/nonmaintainer

See https://www.drupal.org/patch/apply for applying patches.

Ask in #drupal on IRC if you need more help.

Just to say I've tried to reproduce this bug on custom entities using the steps outlined in the issue summary, and the bug doesn't seem to occur there.

I aplied the 7.29 patch and now have many 'wonky' problems with all content types that use the File field. The form that handles the upload will upload a file, show the file, once saved the file is no longer displayed in the GUI. The file still exists on disk but the file is no longer linked in the form. Furthermore all previously linked files in the form are all gone as well, but the files still exist on disk. I suspect there is a bigger problem then specifically with Taxonomy, Files and Images. This is crippling my site, have to revert to 7.28.

Updating based on comments and my own experience.

It definitely affects images attached to file_entities. I have experienced and resolved the issue with patch #42.

I just wanted to confirm that the patch in #42 solves the problem and fixes the bug with file and image fields on taxonomy terms.

I have a somewhat different issue.
I attempted a raw install of D729 in my localhost.
I never had a problem with other versions of Drupal, including 728.
Created a vhost as usual and to my surprise could never access the admin area.
Drupal was consistently rejecting the combo username/pass.
I tried everything several times: deleted all files, dropped database, cleared browser cache, tried different browser, different user/pass combo, attempted to set pass for user 1 with drush, used SQL queries.
Bottom line: I tried all the arsenal for hours and nothing could get me into my own server.
Finally got to 729 by upgrading via drush a 728 site.
I am using the testing branch of Debian (jessie) with apache2
Linux papimalo 3.14-1-amd64 #1 SMP Debian 3.14.12-1 (2014-07-11) x86_64
PHP Version 5.6.0RC2
Could it be that my PHP version has anything to do with this?

@ckosloff I recommend you open a new issue on that in order to keep this one on-topic.

Any pointer of how to test file entities with #42? I saw a mention of there is a problem, not what the problem was.

#42 doesn't work, with custom content types using IMCE as an image browser.

To satisfy the trolling of @scor, given to over whelming amount of data provided on this topic. I have a vanilla install of Drupal 7.29, with IMCE and WYSIWYSG installed.

Create a content type of FIELD TYPE : IMAGE, you will see that creating content will result the same issues described in this topic prior to this post.

@figtree_development you will need to provide detailed steps to reproduce so others can debug.

FWIW, did a manual test with vanilla 7.29 (standard installation) following steps outlined in OP. Confirmed:

* problem exists with field type IMAGE and field type FILE attached to taxonomy term (in my case there was no need to edit the term; simply resaving loses the image)
* problem does NOT exist with these field types attached to a node entity
* patch in #42 resolves these problems with no observable errors

To satisfy the trolling of @scor, given to over whelming amount of data provided on this topic.

Before this patch get committed to Drupal 7 core, we need to test as many scenarios as possible to make sure it works and doesn't break anything else. If you want your bug report to be useful, it has to be detailed enough that it can be easily reproduced, especially if it involves contrib modules where we need versions, settings, etc. Please read these tips. Providing incomplete instructions is what will make this issue overwhelming because others will have to ask you questions.

I have a vanilla install of Drupal 7.29, with IMCE and WYSIWYSG installed. Create a content type of FIELD TYPE : IMAGE, you will see that creating content will result the same issues described in this topic prior to this post.

These instructions are not sufficient to reproduce the bug, and developers will waste their time trying to guess what you meant. That's exactly what I did for the last couple of hours, trying setting up IMCE and WYSIWYG in the hope to see the bug you're talking about, but no luck. For example, Create a content type of FIELD TYPE : IMAGE doesn't make any sense to me. Here are more questions for you to overwhelm this issue a bit more: did you install the imce_wysiwyg module? did you use CKeditor, TinyMCE, or some other editor? If you don't want to write a bulleted list of steps, at least provide a screencast or some screenshots.

@scor

I just wanted to confirm that the patch in #42 solves the problem and fixes the bug with file and image fields on taxonomy terms.

Agreed: In my specific case, the patch in #42 also appeared to fix it. Patch applied fine.

Test was not on a fresh Drupal 7.29 install, but a dev version of a production site. Everything is working on this site AFAIK after the patch in #42.

I suffered this issue on a site where images uploaded to entity bundles through the core image field were dissapearing after node/edit --> save. I applied the patch from #42 and the issue seems to be resolved. When I edit and save, images no longer dissapear.

Thanks everyone for the testing. I did a lot more testing of #42 in various scenarios using contrib modules:

1. Media and File Entity: These seem to work well (both the 7.x-1.x and 7.x-2.x branches).
2. Imagefield Crop: Turns out I couldn't reproduce the bug earlier because I was testing the 7.x-1.x branch. It's only reproducible on the 7.x-2.x branch of Imagefield Crop, which provides a separate "Image Cropped" field type (#2306001: Imagefield_crop field content deleted after entity save after upgrade to Drupal 7.29). In that case it does indeed occur on all entities (including nodes). However, the patch in #42 seems to fix it.
3. IMCE: I eventually figured out that this needs https://www.drupal.org/project/imce_filefield in order to reproduce the problem mentioned in the above comments. (Which indeed would have been useful info for someone to provide up front...) There's an issue about it in the IMCE queue also: #2305499: Selecting images from the IMCE browser results in error message. The bug is reproducible by installing that module, then editing the image field on articles and checking the box to have it use IMCE. When you go to add an article and use the IMCE browser, the images you add don't get attached. The patch in #42 does not help with this. I don't really know what to do about that, since IMCE's hook_file_download() implementation for private files is kind of brute force. I'm posting a workaround patch in that IMCE issue though.

I feel like #42 may be good enough to release this week given that we've fixed most of the pressing issues. We need to balance getting a perfect fix with getting a new release out.

The one thing we could still do in core (which would help with #3) is not call file_download_access() on public files. This is similar to what someone suggested above (although the suggestion there was to change file_download_access() itself). From a security perspective I think this is as far as we can go, but I think it should be safe as long as we only exclude public files? This will allow sites using public files to bypass any remaining bugs, although those bugs would still exist for private files.

This is implemented in the attached patch, along with changes to the test to reflect that.

Any thoughts/reviews on the patch here vs. #42? I'd like to get a release out this week (ideally even tomorrow).

-        if (file_download_access($file->uri)) {
+        if (file_uri_scheme($file->uri) == 'public' || file_download_access($file->uri)) {

If I'm not mistaken, I think this could end up causing the same problem of removing private files if the user does not have access to that particular file. That is, the user had edit access but not view access to the node being edited.

From a performance stand-point, I think this is much better, as checking file access can be expensive. In a multi-file situation, you need to do this for each file individually. So perhaps the ideal fix is both approaches, skipping the check for public files but also using the default if a user doesn't have access to the file specified by the default.

If only one patch were used instead of making a new one that combined approaches, the one in #42 seems like the most appropriate.

@quicksketch, thanks for the review!

Is it possible you reviewed the interdiff thinking you were reviewing the actual patch, though? Because #64 actually already is a combination of #42 and the code mentioned in your comment.

By the way, I also just went and updated the Drupal 7.29 release notes with more details on the bug based on recent comments here, and I posted https://groups.drupal.org/node/434473 to say that we're going to release something this week .... but that we could use more reviews and testing in this issue.

I have the problem on a site where authenticated user can post nodes with pictures (field type "Image") and there is a workflow where users with proper roles (editor) can be publish them after reviewing the contents.

So when the editors click on save, the picture field gets emptied (because the picture is a required field the node cannot be saved). All other fields keep their values.

It's interesting to note that as an administrator of the site I can save the picture node (the picture stays).

The problem appeared after updating from 7.28 to 7.29.

The patch from #64 (file-download-access-2305017-63.patch) removes the issue.

Thanks for all the work.

Does any of this need to be backported to D6?

Does any of this need to be backported to D6?

@drupov, thanks for the testing! Were you using either of the modules mentioned above (IMCE for FileField or the 7.x-2.x branch of Imagefield Crop) that have this issue when images are attached to nodes? (I'm just wondering if there's another scenario that should be mentioned in the release notes. Not too big of a deal given that the patch fixed the problem, though.)

@yktdan, not in core, I believe, but it looks like FileField for D6 is affected with a similar issue: #2305969: "Referencing to the file used in the field is not allowed" error after 6.x-3.13 security update

I'm also reuploading the patches from #64 (with no changes), since it looks like the testbot is stuck on them.

This appears to be a somewhat related issue.

I think this is another related issue.

Why not do a foreach in the test case instead of 2x testX methods that each install Drupal?

I can confirm that the patch at #72 fixes the problem for me, using File Entity & Media modules with image fields on nodes and field collections. My specific manifestation of the issue is described in #2308737: Error on uploads after Drupal 7.29 security update

Is it possible you reviewed the interdiff thinking you were reviewing the actual patch, though? Because #64 actually already is a combination of #42 and the code mentioned in your comment.

Oh, right. Yes you're correct @David_Rothstein. :) New version is great then, combining both approaches already. From a code-standpoint the fix looks good to me.

Why not do a foreach in the test case instead of 2x testX methods that each install Drupal?

I thought about it, but in that case the first test case would somewhat pollute the second (or at least it would without further refactoring). There would be two file fields in play the second time, one private and one public. So I thought it was cleaner to keep the tests separate.

Alright, I think based on the above comments, we can consider this RTBC and release it tomorrow. Still very interested in further reviews, testing, etc. before then though, in case we uncover something else.

(I'm also following up on some of the other issues mentioned above and adjusting the "known issues" section of the release notes accordingly. I had noticed one of the linked issues in the queue earlier, but not the others.)

Thanks everyone!

Argh. The fix works with public files, but I still can't upload a file to a private filesystem.

The site (necessarily) implements hook_file_entity_access() and hook_file_download() for private files. Investigating.

In hook_file_download() I'm checking that the file is actually attached to a node, and denying access if it isn't.

Obviously this can't work during upload because the file isn't attached yet. So now I need some context to tell me whether the hook is running in upload vs in download.

Setting back to RTBC, but mine can't be the only site that does something like this. Any thoughts?

Hm, well the core File module deals with this situation by always granting access to the file owner if the file is temporary (i.e. in the process of being uploaded); see file_file_download(). But if your module is denying access in those situations, it will override the File module's grants.

So maybe if you put something like this at the top of your function it will work?

// Don't deny access to users viewing their own temporary files (for example,
// new files in the process of being uploaded).
if ($file->status != FILE_STATUS_PERMANENT && $file->uid == $GLOBALS['user']->uid) {
  return;
}

Just figured that out myself and it seems to work, thanks.

I think this is a fairly serious change in behavior that needs documenting for hook_file_download() at least. Nobody will be expecting that to be called when a file is uploaded. And maybe it's worth a change notice for D7?

(addition)

The strict check on $file->status will fail when $file->status is the string "1", which is definitely possible. So in my case the check needs to be:

/**
 * Implements hook_file_download().
 */
function MYMODULE_file_download($uri) {
  if ($file = file_uri_to_object($uri, TRUE)) {
    // Allow access to users viewing their own temporary files.
    if ((int)$file->status !== FILE_STATUS_PERMANENT && $file->uid == $GLOBALS['user']->uid) {
      return;
    }
  }
}

I agree it needs documenting, and maybe a change notice (I mentioned it in the Drupal 7.29 release notes for now). See #2308347: Document that hook_file_download() can be invoked during content edit file uploading for more on that.

Although as mentioned there, the Entity API module which is used on half of all Drupal 7 sites already invokes this hook outside of the file download context (though not specifically in the file upload context).... the hook is basically already about determining whether the current user is allowed to download a file, and now core is specifically calling it that way also.

Just a thought: could the the $file->status and $file->uid check be run before module_invoke_all('file_download') is called? So users always have access to their own temporary files, regardless of what implementations of hook_file_download() say about file access?

Just a thought: could the the $file->status and $file->uid check be run before module_invoke_all('file_download') is called?

On that same vein of thinking, maybe we could simply check if the current FID matches the default value, and if so, skip all access checking. Since uploading a file will result in setting the default value (I think), that would eliminate the need to check any access on newly uploaded files. It would also prevent any existing files from being deleted on edit if the value was unchanged. Essentially this might be able to remove all the $force_default business and just say, "if the value hasn't changed, don't check access".

@ cilefen

Thank you, now I start to understand the use of git. I must learn much more about it, I think.

However, only to advise the patch #72 works for me too.

Patch tested on 7.29 with Entity & Field translation and it works, thanks :)

Patch in #72 tested + working w/ 7.29 for image field on taxonomy.

OK, I went ahead and committed this to 7.x, and Drupal 7.30 will be released later today. Thank you everyone for all the help and testing with this issue!

Just a thought: could the $file->status and $file->uid check be run before module_invoke_all('file_download') is called?

Possibly, although it would make things inconsistent with contrib modules (like Entity API) that invoke that hook directly. And it would mean that the behavior could never be overridden. But seems worth discussing in a followup issue?

On that same vein of thinking, maybe we could simply check if the current FID matches the default value, and if so, skip all access checking.

Possibly; I thought about doing something like that but the code for dealing with the default value later in the function is complex enough that I didn't want to deal with repeating it or changing it. I think you're right this would be a small performance improvement. Also a followup issue?

I think you're right this would be a small performance improvement. Also a followup issue?

Fair enough, getting out this initial fix in a timely manner is appreciated. Good call on going with what we've got already and tidying it up in a followup.

after updating to D7.29 almost all existing images have disappeared, either attached to nodes, in views or otherwise. See itpgrfa.net/upgradetreaty

Isn't there an issue if other uri schema's are used for the public file system? Eg hash wrappers, these files are now going to be subject to access checks eventhough they shouldn't be? I am thinking that something like

file_uri_scheme($file->uri) == variable_get('file_public_schema', 'public')

or may even

in_array(file_uri_scheme($file->uri), variable_get('file_public_schema', array('public')))

After additional discussion with my colleges it does seem to be an issue as the code path you end up executing is for private files when the files in question are not private. Array version of patch attached as it provides most flexibility.

Looks good to me. While 95% of sites probably use public:// there are some that use something else (as @marcingy already mentioned in #96).

That looks reasonable; could perhaps use a code comment explaining the purpose of the variable.

Wondering what the specific bug you ran into is though? The original bug reported here, at least, should no longer be possible regardless of which URI scheme is in use.

Retested the patch since it applied fine for me.

Comment added.

Looks good.

If we're inventing a new config variable, shouldn't it get documented somewhere?

Setting back to RTBC as there is no place to document settings without a UI sure something could be added to settings.php but variables such as taxonomy_override_selector are not documented in there. This is a feature that will be used by a tiny edgecase of sites similar to the aforementioned variable.

+1 for #110.

> there is no place to document settings without a UI

I was thinking:

- in the docblock of the function that invents it
- in the change record

Any chance that we could move this patch forward for release? This problem totally breaks Media file uploads on sites with my module S3 File System installed, because it puts files in the s3:// schema.

I've already updated S3 File System to make it register "s3" in the file_public_schema array, so all that's left is for this patch to go live in the next Drupal core release.

Looks like this was RTBC then went needs work based on a failed test but then the re-run of the test went green so should it be back at RTBC?

Despite the fact that the testbot is stuck, the patch still applies cleanly.

Also, should this have new tags for the next core release?

@DamienMcKenna
No, I don't think so. The release blocker tag probably isn't necessary anymore, nor should it be necessary for this issue to block another release since the regression has been fixed, I think. The existing release notes tag has to stay so it can be found by anyone looking for patches applied to 7.30. A new release notes tag can't be added until we know what version it will go into, which we can't assume since there could be a separate security release before the next bug fix. So the tags can stay as-is or the release blocker tag might be deleted.

Setting #104 back to RTBC

I thought this was needs work for a while for the documentation... some people above felt it was not complete? I tend to agree; given the security implications of this variable we should document it very carefully.

I did not really see another good place to document this, though, so in the attached I'm just modifying the inline code comment.

I will leave this at RTBC since all I changed was the code comments and it's been there a while and it would be good to get this one committed. A quick followup review would be welcome though.

I also confirmed that nothing from this issue really needed to get into Drupal 8; however, the tests that were added here would be useful in Drupal 8 (even though the regression was never present there) so I filed a followup issue about that.

OK, if anyone really doesn't like my code comment changes, we can always follow up with another patch to change them, so...

Committed to 7.x - thanks!

Restoring the issue metadata from the original issue, since this commit was just a small followup.

I know this issue has been closed but I'm still having the same problem with Drupal 7.38. In my case, my attached files are NOT Images nor taxonomy and does NOT use IMCE (these are all things mentioned above in others' comments). My file field is a simple unlimited File field named filed_video_files.

Just to repeat what the problem is, when editing a node with attached files (the files do not actually change), after form processing, the attached files are gone from the node!

My solution, since I couldn't patch core is to add a form validation in the edit form (via hook_form_FORM_ID_alter). In this validation, I simply update the info:

 function mymodule_edit_form_validate ( $form, &$form_state )
 {
     $form_state['values']['field_video_files'] =
         $form_state['node']->field_video_files;
 }

Hope this helps someone else!

We just experienced this too, running Drupal 7.36.

A Commerce product display node was edited along with its product via Inline Entity Form. After saving it, the image (poster) for the product as well as our product files (which are attached via Filefiled Sources and served via S3) were removed from the node entity. Their records were removed from all database tables as well (e.g. field_data_field_poster).

Judging by the comment in #131 it seems like this issue may still exist - and the form_validate fix in that comment is not really feasible for a site like ours that has a large number of fields. If another Issue should be opened to address this apparent regression I will do that.

Possibly related: #1141912: Changing node language by moving down language list deletes files in file field

FYI, I upgraded to D7.41 and the problem still exists and is reproducible

@monaw: Can you tell me more about your setup? Can you reproduce this on a new Drupal install (in a minimal install)?

I am also on D7.41 and still getting this issue. Mine is not a clean install, though.

@shivams, this patch has been commited. So, this issue is fixed and closed. Please, update your Drupal core version.