When an issue with file uploads on them is re-edited, the use of unset($form['attachments']) causes data loss whereas $form['attachments']['#access'] = FALSE would allow the data to be passed through to form submit without modification.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Dave Reid’s picture

Status: Active » Needs review
FileSize
582 bytes
dww’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks! Lame bug. Definitely confirmed locally. However, this patch doesn't fully solve it.

A) If you create an issue with an attachment, then edit the issue and create a new revision of the issue, you still can't see the attachment. Closer inspection reveals that the attachment is still in the DB, so there's not data loss per se anymore. But, the attachment is still invisible once you edit the issue (unless you revert to the original revision). Is there another issue about this? I see #1178296: All file attachments regardless of revision displayed on node view but that seems like a different problem. Am I confused?

B) We really need simpletests for stuff like this, see for example #1178318-2: Add "edit any project issues" permissions. It's 3:15am here, and I've never had to write a test that uploads files, so I don't really have the brain bandwidth for that right now. ;)

Thanks!
-Derek

Dave Reid’s picture

Hrm, I could not replicate the same behavior. Creating a new revision with the patch had the attachment show up just fine (since upload.module adds a new record with the new vid, but using the same file record). I'll try to get a test or two written. I'm more hesitant about tests that involve uploads.

webchick’s picture

I was able to reproduce dww's #2A with user jhodgdon / pass awesome on http://metrics-drupal.redesign.devdrupal.org/node/1165697, which has Dave's patch applied.

http://drupalcode.org/project/simpletest.git/blob/refs/heads/6.x-1.x:/te... might have some code that could be poached for this test.

webchick’s picture

Interesting. So you can't reproduce this bug on davereid's sandbox http://davereid.redesign.devdrupal.org/ which was last re-imaged 14 weeks ago, but you can on metrics http://metrics-drupal.redesign.devdrupal.org/ which was last re-imaged about a week ago. Which implies that this bug was introduced sometime between just before DrupalCon and now.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
966 bytes

AGH!!!!!! I 'fixed' the wrong section. I fixed the comment file attachment field, but not the *node* file attachment field (but I had it applied locally on my sandbox, hence the confusion as to why all of a sudden it regressed with this patch).

webchick’s picture

This still needs tests, but I can confirm that #6 is tested and working. YAY!

dww’s picture

Status: Needs review » Needs work

Still needs tests, but it's a working fix for an important bug, so I committed and pushed:

http://drupal.org/commitlog/commit/1894/292f1c56a3d248c0e078287b957513af...

Thanks!
-Derek

webchick’s picture

OMG. THANK YOU DWW!!!!!!!!