File(s) silently deleted when #access=false

Tagging issues not yet using summary template.

I too was using the multistep module and noticed my images disappearing when saving other steps which did not include the images. When investigating this issue, I came across this patch.

I can report that the submitted patch resolves the issue (no files/images silently deleted from the node).

I would suggest changing the title to "File(s) silently deleted when #access=false" as that more generally represents the issue at hand.

Anyone can change the title. :)

Let's get some test coverage on this. Looks like extending file_module_test_form() in modules/file/tests/file_module_test.module to include an $access parameter might be a place to look.

Tagging for backport.

I tested this patch on D7.12 with the current Multistep dev 2011-June-27, and confirm that files stay saved in multiple steps. As far as I experience now there are no side effects.

Still needs test coverage != RTBC. :)

It's not RTBC until it includes those tests. :)

Edit: Crosspost. :P

Hi. Sorry for failing to report back. I am having trouble making my test fail without the patch D7. Can you share your form axel.rutz?

I'm trying to put together a test case for this, from my understanding we need to use a multi-step form to test whether the patch fixes the bug, my approach so far has been to add another button to the file_module_test_form() form. I've attached my work-in-progress file_module_test.module patch, I know it's not pretty. So far I've just been testing file_module_test.module directly in the browser by removing hidden = TRUE from file_module_test.info.

Here is my testing procedure, after enabling the 'File test' module:

1. Visit file/test
2. Upload a file, then click 'Continue submit'.
3. Once the page reloads, click 'Save'.
4. Verify that the file has been uploaded to the files directory and is in the file_managed table.

With this test case, the patch doesn't seem to make a difference, so I think I'm definitely missing something.

Maybe we could try creating a form with the multistep module, with which the patch reportedly solves the issue, and inspect that form's structure?

We have to simulate the way it does multistep. It's not a core module, so we cannot use it here. Every step in multistep form module is actually a different URL.

Since this is showing up only in a contributed module and so far not replicable by core, degrading to normal.

It would be nice if the maintainers of said module could handle creating the test case. Core developers have so far been unsuccessful in creating it by sticking to common patterns in core, so I'm guessing multistep is doing something a bit out of the ordinary.

From what I saw when testing the Multistep module, the module seems to alter existing forms (it doesn't create forms of its own), and stores the step in the URL query string.

As the original issue stated, the Multistep module sets #access = FALSE for fields not on the current page. The fact that this is done through a field_group.module hook - hook_field_group_pre_render() complicates the issue even further, and makes it harder to reproduce this behavior in a comprehensible test case in core.

It was also pointed out when discussing this issue on IRC that the existing condition (below) was likely written that way for a reason, so we definitely need tests (and comments in the code) to prove that this change is sound.

if ($input !== FALSE) {

I bumped into this problem while creating a block (in a custom module) that grabs the user_profile_form, which contains an image-field and setting this field's #access to FALSE: after submit the previously-set image is deleted.

So I just wanted to state that this does not _just_ happen when using the Multistep module, it happens whenever you set a file-field or image-field to #access FALSE. The test-case would most likely be easier to follow my steps.

function any_menu_callback() {
  $account = user_load('...');
  $form = drupal_get_form('user_profile_form', $account); // Or any other form that contains a file-field (I think, untested!)
  $form['field_image']['#access'] = FALSE; // Any file field that currently contains a file
  return render($form);
}

And now save the form. In the current situation, the image will be deleted.

I can confirm changing ($input !== FALSE) to just ($input) fixes the problem, while maintaining the ability to unset a file with the 'remove' button, so I think this fix is correct.

It seems checkboxes have the same issue...

The fix (removing !== FALSE) does have a side-effect: with an image field that has a default image selected, the fix prevent you from unsetting the default image. Going to look for a better fix ...

No work for a few weeks. Making it unassigned so it can be picked up by anyone. ACF, reassign it back if you want, no worries. :)

typo

I moved this comment into the issue summary.

I did some investigating and found this snippet under file_managed_file_submit($form, &$form_state) around line 1259 in file.module.

  if ($button_key == 'remove_button') {
    // If it's a temporary file we can safely remove it immediately, otherwise
    // it's up to the implementing module to remove usages of files to have them
    // removed.
    if ($element['#file'] && $element['#file']->status == 0) { 
      file_delete($element['#file']->fid);
    }    

Removing the if statement around file_delete() solves the problem with the default images not being removed. But that doesn't seem like a reasonable solution.

Adding new information to the issue summary.

In comment #19, webchick degraded the priority from major to normal as it was believed this was a bug in the multistep module. Setting back to major because this has been reproduced in core.

I'm also unassigning this from myself in case anyone else wants to jump in.

Thanks @dags.

Let's get an automated test for this based on the STR in the summary.

First draft of a test.

Nice work, thanks @dags! That test looks good.

A few suggestions:

1. --- a/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.php
   +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
   
   @@ -20,6 +20,31 @@ public static function getInfo() {
   +    $type_name = 'article';
   +    $field_name = strtolower($this->randomName());
   +    $this->createFileField($field_name, $type_name);
   +    $field = field_info_field($field_name);
   +    $instance = field_info_instance('node', $field_name, $type_name);
   

   Let's add an inline comment above this to make it more scannable, something like "Add a file field to the article content type."

   Also, ideally, we should use a test content type rather than article for better decoupling. (See LocaleContentTest for an example.) If FileFieldWidgetTest does not currently use a test bundle elsewhere, let's introduce it here first and then file a followup issue to use that test bundle throughout this class (or possibly throughout all classes extending FileFieldTestBase, depending).

   In an ideal world, we'd use a test entity instead of node, but that's probably out of scope here. (We wouldn't be able to use uploadNodeFile(), and the test entity class is in flux with the EntityNG conversion, plus #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest.)

2. +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
   @@ -20,6 +20,31 @@ public static function getInfo() {
   +    $test_file = $this->getTestFile('image');
   +    $nid = $this->uploadNodeFile($test_file, $field_name, $type_name);
   +    $node = node_load($nid, TRUE);
   

   Another inline comment here would be good, like "Create a node with an uploaded file in the field."

3. +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
   @@ -20,6 +20,31 @@ public static function getInfo() {
   +    // Save the node and ensure it does not have the file.
   

   I think this should say "and ensure the file is not deleted"?

4. +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
   @@ -20,6 +20,31 @@ public static function getInfo() {
   +    $this->assertFileExists($node_file, t('File still exists.'));
   

   The t() can be removed from around the assertion message here. See: http://drupal.org/simpletest-tutorial-drupal7#t

Working on ticket at Portland2013 sprint. Going to combine patch and test, then reroll after a manual test on my local.

Looks like the patch created before may have gone stale. Initial tests show the test passing with or without the update. I'm going to keep looking, though. As I suspect it may have been human error on my part while testing.

Adding links to related issues

I combined all 3 patches and re-rolled against 8.x.

Will work on other issues based on this patch.

Attached patch includes comments from #34 and #37. I also changed

  if ($input) {

to

  if ($input !== FALSE && !== NULL) {

I have feeling that we can get array() as an input in here which must processed. Initial patch would ignore this case and display default value instead. I may be wrong, so we probably need to investigate this a bit more.

About bug mentioned in #23... It looks like it also exists in HEAD, which means that was not introduced by this patch. I'll do some more investigation and post another bug if I confirm my assumptions.

Confirming default image issue as unrelated bug. Bug report: #2051213: Unable to remove default image from image field/instance.

Formatting issue summary so it follows the template.

Issue summary was updated to reflect latest info.

@slashrsm: Patch looks good. One minor issue:

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
@@ -30,6 +30,40 @@ public static function getInfo() {
+    debug($type);

Remove this line? I'm not sure if coding standards actually require debug() lines to be removed from tests...

Also, could you post the test separately from the patch? So we can see it fail without the patch and pass with it.

I definitely didn't want to leave debug() in there :).

Any other comments? Could we RTBC it?

Update summary with latest info.

I manually applied the patch in #45 and it worked well for me. I was having problems saving images with the Multistep module.

Many thanks!

Hello and thanks,
is correct apply only this?

I'm sorry, I've forget to select D7

We need to fix this for d8 and then backport to d7.

I'm sorry, but I don't work with D8...
I'll try to port this patch in D8 early.

Rerolled. Cannot reproduce manually. Also, this test should fail and works. Anyway, I think we could add this test.

Assigning for testing

Patch needs reroll

Re-rolled.

Issue was assigned in May. Unassigning so it is clear anyone can work on this.

Shot in the dark here..

I've tried applying the patch in the description and it doesn't make a difference at all.

As soon as I press next / previous to go back onto the page I uploaded the file, the file's deleted.

Any help would be massively appreciated.

Tried to reroll this and it was working on my local. Let's hope it would not break in auto testing.

This was a bugsmash target yesterday. Lendude and myself discussed this.

The last report that this problem was occurring and the patch fixed it was in #47, 9 years ago. Then there was a break for 2 years. After that,
in #53 a patch was made to reproduce the problem. That patch passed tests and it was reported that the problem could not be reproduced. The same comment suggest the test could be added. However, if the problem not longer exists, perhaps there is an existing test and this one does not need to be added. Since then there has been no activity.

What remains is to determine if the test is worth adding. I have updated the issue summary.

FWIW, I updated the patch and yes it passes.

And no longer a bug report.

Fixed failed commands on #81
Attached patch against Drupal 9.5.x

@quietone I don't think this issue is definitively closed, as this bug still appear if you set the access to FALSE on a parent element of the file managed element, but not on the file managed element itself.

Actually the point above seems to be caused by late alteration of the form, so this is not related to the current issue.