Summary

When uploading files to a node (Page content type), and unchecking List for all files, the last uploaded file is still listed upon saving the node. In the case of uploading only one file to a newly created node, that file is still listed despite unchecking List for the file prior to saving the node.

Steps

  1. Create a new node of the content type Page
  2. Upload multiple files using upload module
  3. Uncheck List for ALL uploaded files
  4. Save the new node
  5. Last uploaded file will still be listed despite unchecking list for all uploaded files

Expected Result

All files uploaded and having List unchecked for each should not be listed on the node.

Actual Results

All files with the exception of the last uploaded file do not display after unchecking List for all files and saving the node. In other words, if one uploads three files to a node, and unchecks List for all three files, then saves the node, one notices that the first two files are indeed not listed, but the third file is, despite also having List unchecked.

Notes

I tried this a few times, using Firefox 2.0.0.11 on OS X 10.4.11 - a couple times with three (3) uploaded files, and once with just one (1) uploaded file. The result with three (3) files was always that the third file would be listed despite unchecking List prior to saving the node, and the result with one (1) file was that the file was listed despite having unchecked List for the file prior to saving the node.

When one edits the newly saved node, and unchecks List for the file which is still being listed a second time, and saves the node, the file is no longer listed as would be expected.

CommentFileSizeAuthor
#8 upload-list.patch2.5 KBwebernet
#3 upload-list-6.0.dev_.patch1.39 KBgdevlugt

Comments

mariuss’s picture

mariuss commented

Same here.

And it happens not only with newly created nodes, you can attach a file to an existing node, same thing will happen.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical

Reproduced, needs to be fixed before release.

gdevlugt’s picture

gdevlugt commented
StatusFileSize
new upload-list-6.0.dev_.patch1.39 KB

I think this is caused by the the following lines in _upload_form() :

      // If the file was uploaded this page request, set value. this fixes the
      // problem formapi has recognizing new checkboxes. see comments in
      // _upload_prepare.
      if (isset($_SESSION['upload_current_file']) && $_SESSION['upload_current_file'] == $file->fid) {
        $form['files'][$key]['list']['#value'] = variable_get('upload_list_default', 1);
      }

When removing the if statement, the list checkboxes seem to behave properly.

Could this possibly be a remnant from Drupal 5? The only thing I could find on the mentioned _upload_prepare() function is in Drupal 5.

I attached a patch which removes the lines of code mentioned above. I'm not sure if the code is needed in Drupal 6 and perhaps someone more knowledgeable than me on the subject can tell us if the lines are needed or not.

gdevlugt’s picture

gdevlugt commented
Version: 6.0-rc4 » 6.x-dev
Status: Active » Needs review
cburschka’s picture

cburschka
they
commented

This patch fixes the problem for me, both with multiple files and a single file.

I have also uploaded new files with the patch without causing any problems with checkboxes. However, due to the nature of the patch perhaps it should get a second review from someone with Form API expertise.

webernet’s picture

webernet commented
Status: Needs review » Reviewed & tested by the community

This looks sane, and appears to be working fine for previews as well as saving.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

This is already RTBC so no sense in reviewing it, but I just wanted to throw a shout-out to brianshumate for the awesome bug report! Very thorough, clear and easy to understand, as evidenced by the fact that the bug was fixed in about 24 hours. ;)

Thanks!!

webernet’s picture

webernet commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new upload-list.patch2.5 KB

Attached patch also removes other uses of the seemingly unnecessary session variable.

killes@www.drop.org’s picture

killes@www.drop.org commented
Priority: Critical » Normal

It's certainly nice to fix this, but why would anybody think this is a critical issue?

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Needs review » Reviewed & tested by the community

patch works.

gdevlugt’s picture

gdevlugt commented

#9 killes:
I wondered this myself, but it could be considered critical if you use unlisted attachments for documents containing sensitive information.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 7.x-dev

Also noticed this on drupal.hu. Good catch. Committed to 6.x. Now RTBC for 7.x.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

brianshumate’s picture

brianshumate commented

*blush* No problem! Thanks to the awesome dev team for such an expedient fix! I'll see what I can do about being more inclined to submit more bug reports in the future based on this super response. Thanks again everyone!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.