"list" checkbox is always saved as unchecked for new attachments

I know and I think today CVS is fixed, so please do a fresh checkout and check whether the bug is fixed or not. If you do not like it then by tomorrow morning the drupal-cvs.tar.gz will contain v 1.37 of form.inc which, I presume fixes the problem. Use that and please report back.

This problem seems to have been fixed. I cannot duplicate.

I believe this is the same issue.. This is with current HEAD.

• Select create node form. Add content. Select file for upload
• Click attach.. File attached via snazzy JS..
• Current Checkboxes:
  Delete:Off
  List:On
• Click Preview
• Upon Preview, checkboxes change to:
  Delete:On
  List:On
• Click Submit. Node created. File not listed in node.
• Edit node form. File is listed [even though "Delete" was selected earlier].
• Attachment checkboxes now read:
  Delete:Off
  List:Off
• I can then check List to On, and get the attachment to display correctly..

-K

had the problem today in a current cvs head version:
i repost what a wrote i another issue:

There are a few bugs you can experience:
For all of them you have to go (for example) to /node/add/page and then attach a file;

1. uncheck the List checkbox
click on preview

Result: delete and list are checked and you cant undo this bug
Try this if you have more then one file in your File attachments list... (alle checkboxes are checked)

2. say you uploaded accidently the false file
What would you do to undo thes change?
I would check the "delete" checkbox
But if you Submit the new node then the file does not get deleted
but it will not be listed

btw: I think it would be nice if we would add a description for the checkboxes
Eg. "All Attachments where list is checked are shown to the User in a small Table under the Content"
and/or "All Attachments where delete is marked will be deleted after you press Submit"

Also see http://drupal.org/node/46886 and http://drupal.org/node/47660.

The problem is that the list parameter is set to the fid. When a new attachment is uploaded, it doesn't have an fid, so list is set to 0.

This is a critically needed fix for 4.7 release. While there is a workaround this is key functionality and will reflect poorly on the Drupal project.

The problem is that the list parameter is set to the fid. When a new attachment is uploaded, it doesn't have an fid, so list is set to 0.

Yup. There is no file id to associate the new uploads to until after one is generated when first inserting them. Here's a patch that updates the values based on what is passed in at the time (namely the list array contains stuff like ([upload_1] => upload_1, [upload_2] => upload_2, [upload_3] => 0) which can be used to figure out what the setting should be.)

I don't think that's a good fix. We should correct list so it's a BOOL, rather than the fid.

This will put the right value in the table. Maybe someone can come up with a more elegant solution? Maybe change the form values to return 0 or 1 instead of 0 or 'upload_#'?

A unified patch file instead of the raw diff from last time.

there is a patch, right?

there is a patch, right?

No offense, ln, but -1 on this patch. We should fix the root problem, not hide it.

junyor is an authority on upload matters: needs work

Could someone please look into this? it is a legitimate critical bug AFAICT.

I'll pick it up... I think I know whats going on.

I'll pick it up...

This patch has some major changes in it...

1) the list value case for this issue is fixed... and also another issue I found with check boxes has been worked around to set the list value on a direct submit w/o attach or preview. Extended notes in comments.

2) upload_nodeapi op = validate has been spread across prepare and validate hooks, and moved to _upload_prepare and _upload_validate.

3) multiple upload_load calls have been eliminated.

4) _SESSION['file_uploads'] renamed to _SESSION['file_previews'] to remove the interaction between the fileapi
and the upload.module via the session to help decouple the two.

5) the file form has been redesigned to mimick the node->files structure so the form API can handle some of the heavy lifting. (minus the checkbox on submit bug).

I know this change seems to go beyond the original issue, but changing the form structure was the best way I could think to eliminate this bug, and let the FAPI do the work. The rest of the changes flow from debugging that change.

I may be able to role a patch with less changes, but I would really like to see the upload module get some much needed cleaning up before 4.7rc.

.darrel.

btw. The above patch depends on the last patch I posted to http://www.drupal.org/node/5961 as of 3/7/06...

My time is a little limited this week... but I'll do what it takes to see this patch through. so please post your feedback.

And the earlier patch is the wrong patch. File deletion doesn't work.

dopry, awesome job!

However, running into a problem on Windows, my uploads aren't going into the right direction, it is say right below the file:

http://localhost/drupal_devel/drupal/files/c:/windows/temp/tmp68C.tmp

instead of:

http://localhost/drupal_devel/drupal/files/tinymce.patch

I think the incorrect path is coming from my file_check_upload changes.

I haven't experienced the same issue on linux...
Does the something similar happen before applying this patch, but after the file_check_upload patch from http://www.drupal.org/5961?

After some playing m3avrck and I figured out there was bug with the _upload_form: $description = '' line.
So here is the updated version.

Thank you for your help m3avrck.

.darrel.

Updating status...

The patch has a minor problem -php tag at the top- but once fixed it works.

Not that I understand all the code in upload module - actually I feel a bit sick every time I try to :-( - so all I've done is apply the patch and test. Works as expected.

This looks like a really complex patch, for what looks like a simple problem. Sure this is the right approach?

I think it is the right approach. I agree it oversteps the original issue.

1) it solves the issue.

2) I think it make upload.module much easier to understand.

3) It addresses several issues that make upload.module difficult to debug.
--the multiple upload_load calls.
--the unintuitive node->list and node->remove

4) properly uses nodeapi prepare and validate. To bring interaction of contrib modules
with the core upload process closer to reality... (You can do a lot with module weighting and a new theme function for the current files form).

I strongly believe it is the best solution. (I wrote it so why shouldn't I.) I think the current upload.module is very difficult to understand, while my patch doesn't make it a childrens novel, I think it is a step in the right direction that will mean it easier for people to understand the current upload process (which hasn't really changed). It delineates the upload process in concise steps with better terminology that people are familiar with through nodeapi. It is better documented.

I would rather have an upload.module I can understand, than one that has received another piece of duct tape.

I will even keep myself on tap to fix any issues that arise as a result of this patch.

If the naysayers are loud and well spoken I will figure out where to apply the duct tape. I really want to see 4.7rc asap, which I was I have my own reservations regarding this patch and the depths of its changes, but I also abhor upload.module in its current state.

.darrel.

Thanks for the comments.

I think I would be OK with this patch. However, parts of the patch are hack-ish (like the way you use sessions). That stops me from committing it. That and the size of the patch; it takes a while to grok it properly.

Also, FAPI is not an official term. Better to refer to it as "forms API".

Which use of sessions? The checkbox issue, or for storing upload_previews before submission across multiple page views?

The checkbox issue I have not been able to think of a way around, except for fixing for building to do the same thing... I went over that issue with hunmonk as well in IRC and he coudln't think of a better solution either.

For tracking uploads, it is the way it was done historically, and I think may be necessary to get the information into hook_menu to build the callback for previews, but I may be incorrect there. I didn't question that part I just kept it intact within the upload module, and changed the key so there would no longer be any hidden interaction with file.inc.

If you will expand on how the use of sessions is hack-ish I will address it.

I had a look through the patch and quite like what I see. The code is now much clearer and cleaner. Excellent work, dopry.

I noticed a couple lingering todos notes that need to be taken care of. What needs to be done to the delete functions? There's also a note to add the default new file list behavior to the file settings page. However, isn't file->list already set at that point (in upload_save)? I'm not opposed to a setting, but we should use it when file->list is originally set in _upload_prepare.

Isn't "file_check_upload('upload')" more or less meaningless or have you changed that recently? If it still means nothing, I'd like to see a better call to file_check_upload() in _upload_prepare().

Other than these issues, I noticed a couple style problems ("strpos($fid,'upload')" instead of "strpos($fid, 'upload')"). I'm attaching a patch with some documentation updates, too.

It looks like there's only one call to file_check_upload() left (in _upload_prepare). If _upload_prepare is only called once, this patch should also solve the mime magic bug.

What needs to be done to the delete functions?

I think the delete functions are fine as they are.. I just see very similar code in all of them that I think could be reduced into an _upload_delete_file().

However, isn't file->list already set at that point (in upload_save)?

Not it is not set. This is where the checkbox bug I mention sets the checkbox value to 0.
However I should use the variable_get('upload_file_list') in _upload_prepare If I am going to add it to the settings.

Isn't "file_check_upload('upload')" more or less meaningless

It has more or less the same meaning as file_check_upload().

I left the @todo's seperate. I felt I was already introducing enough change with this patch. I didn't want to push it.
I felt those changes could be more easily seperate into their own patches, and were just sideline thoughts I had while working on the listing options theory...

I addressed Junyor's issues with the code style,
remove FAPI as per Dries comments,
added the setting for default listing behavior that was a todo,
removed the todo's regarding refactoring the deletes.
fixed my comments to use the word hack less, and explain my use of session['file_submitted'] more clearly.

@Dries... I would still like verification from you about which use of _SESSION is hackish or if it is both, and why it is so.

@dopry: Could you merge in the documentation updates I made?

@Junyor as far as I can tell I have. Are there any you see that I missed... I just glanced over both patches again, and didn't seen anything.

#32 Dries > This looks like a really complex patch, for what looks like a simple problem.

I think this is the question. But that is just a consequence of the upload mechanism and the upload module being quite a complex thing.

IMHO what we need, if not for Drupal 4.7, maybe for the next one, is to rewrite the whole thing and to get rid of session variables, using instead the files table -with temporary entries that can be cleaned up on cron later.

The thing is, using session variables to keep track of temporary files in the filesystem has the potential -quite real I'd say- risk of letting forgotten files around. Another issue, is that, using temporary names for files that are kept in the session variables may cause clashes if two users try to upload a file with the same name at the same time.

About this patch, I say +1 as it fixes the problem for now and is not more hackish than the upload module itself. But I'd like to think of something better for the long term.

The abandoned files in Drupal's Tmp directory is something I noted in my file_check_upload patch. Before that patch landed files stayed in php's upload_tmp_dir until file_save_upload was called. Since php has its own garbage collection system it will clean them out. I have a patch that implements a similar garbage collection for drupal.

Filenaming is a non-issue, since filemove is called with $replace = FILE_EXISTS_RENAME in file_save_upload.

@dopry: I changed the following: "Clean up old file previews if a post didn't get us to this page. IE) we left the edit page, because we didn't want to upload anything."

to:

"Clean up old file previews if a post didn't get the user to this page, i.e. the user left the edit page, because they didn't want to upload anything."

Here you go...

There was an error in upload_save. I wasn't make sure node->files was an array before using it in a foreach loop.
Here is the update.

There are a lot of changes which, for obvious reasons, make people nervous coming up to 4.7rc. So, therefore I will do comprehensive testing on this. For example, I will be trying various file types, extension-less file types, corrupt images, and so on. I'll check out personal and public uploads by various users types. I'll browse the hooks and create situations to test them as well.

Any specific requests about functions that should be tested?

#45 - Also, just to be sure, is this a unified patch to be applied to HEAD?

Yes it is a unfied patch against head.

applied the patch and browsed to node/add/story. got an immediate error: Missing argument 1 for file_check_upload(). the backtrace (with function arguments) is below. read it from the bottom up. we probably don't need to check_upload when first diaplaying the 'add' page:

9 node_invoke_nodeapi(class stdClass {var $uid = '1'; var $name = 'weitzman'; var $type = 'story'; var $created = 1141960087; var $date = '2006-03-10 03:08:07 +0000'; var $comment = 2}, 'prepare') /Library/WebServer/Documents/dpr/modules/node.module:1669
10 upload_nodeapi(class stdClass {var $uid = '1'; var $name = 'weitzman'; var $type = 'story'; var $created = 1141960087; var $date = '2006-03-10 03:08:07 +0000'; var $comment = 2}, 'prepare', NULL, NULL) /Library/WebServer/Documents/dpr/modules/node.module:317
11 _upload_prepare(class stdClass {var $uid = '1'; var $name = 'weitzman'; var $type = 'story'; var $created = 1141960087; var $date = '2006-03-10 03:08:07 +0000'; var $comment = 2}) /Library/WebServer/Documents/dpr/modules/upload.module:314
12 file_check_upload() /Library/WebServer/Documents/dpr/modules/upload.module:18

oops. my checkout was stale ... i tested this patch and it certainly fixes the critical bug. code style looks good, RTBC

Committed to HEAD. Great job!

i am seeing the list checkbox disabled by default for new uploads. i had never changed that setting on admin/settings/upload, and the admin page said 'enabled' for this. i then explicitly set this to disabled and then enabled and drupal behaved as epected. i wonder if there isn't a bug here in the case where the variable is not set.

Please open new bugs in new issues.... I don't want to confuse a settings problem with the changes made to upload.module.

I created a new issue http://www.drupal.org/node/56555 for thsi settings issue.