Uploads not Working/Preview Not Working

A few things:

1. no tabs :-)

2. patch is off by 8 lines

3.

    case 'validate': break; 
    

Looks fishy.

3. While patch looks good and does work, it attaches files to *all* nodes, that can't be good.

Ok, code looks a lot better and node attaching works too!

One little bug I found, if you have files attached, and then edit the node, and change the status of one of the files (say unlist or mark for deletion) and then add another file, those changes aren't remember during the JS upload.

Other than that, this patch is working great and uploads are *finally* working :-D

I can confirm that in Internet Explorer 6 uploaded files are attached to all nodes but in Firefox 1.5 it works fine. I suspect a javascript issue but don't know enough to debug it properly.

Using the latest CVS version??? Uploads don't work even with JS disabled.

Patch no longer applies cleanly.

Ok here's a patch that applies cleanly.

upload.module_11.patch applies, 12 not.

Preview works, file gets into the temp dir. Submit partially works; the file is still in temp dir and NOT in files dir. A db search in phpmyadmin shows that the file only exist in the session table. so if the sessions gets wiped so does the upload. the url files/file.foo works even if its placed in the temp dir.

When uploading attachment while making a new node you cant "undo" a uploaded file, the delete checkboxes doesnt work.

hmm i guess thats everything for now.

keep up the good work!

Lot's of these warnings using PHP 5.1.1

Warning: Call-time pass-by-reference has been deprecated - argument passed by value; If you would like to pass it by reference, modify the declaration of [runtime function name](). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file. However, future versions may not support this any longer. in drupal\modules\upload.module on line 256

@saerdena did you check your PHP settings? sounds like an issue if the file can't be copied from temp to files/ . Maybe no write permissions? Does the directory exist? Are you settings correct in admin > settings? I'd check those, as I cannot recreate that issue either.

Note this issue fixes this one: http://drupal.org/node/37605 or addresses it. Seems like that one is a duplicate or older issue now

Note, this patch should also address this issue (which it does partly): http://drupal.org/node/31102

Seems the postback problem is due to the JS based uploader *only*. However, with JS disabled, list should still be enabled by default, otherwise it works fine. Damn JS!

It seems this patch http://drupal.org/node/33209 never went in either. The patch above should not be adding this.

yeah im sure there's write permissions because if i change the files and temp dirs the new ones are created by drupal.

I was about to test the latest patch but it fails to apply. hunk errors :\

Patch doesn't work because nodeapi change went in, please update!

Just reading the code , I have a suspicion: $form['current']['#tree'] needs to be TRUE and then a lot of lines can be ommitted as this property is inherited if not set. It's also possible that you will want to set $form['current']['#parents'] = array()

The comment about NULL the sesions uploads when editing isn't too clear. Why exactly is this necessary?

That comment still doesn't make it too clear, at least to me.

The code doesn't seem 100%... when using non-JS upload, or when previewing after attaching a file through JS, any new files are not listed in the table. But they do get attached correctly when you submit the node.

Also, JS delete doesn't really match with the rest of the node workflow. Nodes are not altered unless you hit submit, as a rule.

Previews and subsequent attachments play nicely again. When I tested this on my NT/IIS install, it broke oddly -- treated the file as http://www.my.server/_0 instead of the correct filename. That one's dubious though, as the server is currently being hacked and tweaked. Will investigate more, but on LAMP things are all happy.

that _0 is not related to this patch I saw it reported against 4.6.x too in the bug queue http://drupal.org/node/39805

Looks like no complaints from anyone, let's get this in!

I concur. +1 it works yay.

Thanks for pointing out that other issue, chx. Hadn't spotted it and hadn't been uploading files on that particular server until I tested this patch.

It works great. I have tested, but not found any new bugs.
The attached patch is rolled from drupal root.

I don't think we need another issue, it is only a few lines to throw it in this patch and it is 100% related.

Also, count() <=1 in the query, that should probably just be changed to LIMIT = 1 instead, should be faster.

Ok for some reason that got all cut up. Anyways the query for 'delete revision' should be changed to LIMIT=1 instead, don't need to use the count and compare it to 1.

On second though, disregard above, I misunderstood the reason for the query after talking with Souvent22.

I read through the code and it looks OK. I suggest unset($_SESSION['file_uploads']) instead of setting to NULL. Also consider renaming that to upload_files or somesuch so you prefix the variable with the module name. A minor point.

In Firefox if I attach a file then hit Submit (not Attach) I get warnings:

* warning: mime_magic: invalid mode 026763477601. in /home/mel01/public_html/drupal47/includes/file.inc on line 150.
* warning: rename(/tmp/phpKp3QuC,/tmp/phpKp3QuC.txt): No such file or directory in /home/mel01/public_html/drupal47/includes/file.inc on line 160.

In Safari when I hit submit I see:

* warning: mime_magic: invalid mode 026763477601. in /home/mel01/public_html/drupal47/includes/file.inc on line 150.
* warning: mime_content_type(/tmp/phpfqtcoc): failed to open stream: No such file or directory in /home/mel01/public_html/drupal47/includes/file.inc on line 150.
* warning: mime_magic: can't read `/tmp/phpfqtcoc' in /home/mel01/public_html/drupal47/includes/file.inc on line 150.

And in Safari when I hit Attach I see:
* warning: mime_magic: invalid mode 026763477601. in /home/mel01/public_html/drupal47/includes/file.inc on line 150.

The files to seem to be uploaded correctly though!!

This will be FAQ, I am afraid. Your server is not configured correctly, you have a problem with your MIME magic file.

This thing has been thorougly been tested and I can confirm it is now working with revisions.

After this patch goes in, the issue queue needs to be cleaned up as there are quite a few issues complaining about broken uploads.

When I tried uploading a file, I got htmlspecialchars() expects parameter 1 to be string. I also got The selected file could not not be uploaded, because the destination files/test.txt could not be found, or because its permissions do not allow the file to be written.. I did not set the proper directory permissions so the htmlspecialchars() problem might be related. Please check.

Dries that is an issue with core, not with this patch. There was a thread a while back, http://drupal.org/node/26249 , which fixed the temp problem.

I brought up the idea of auto creating a temp folder if you had *not* been to the settings page but this was shot down because setups very *too* greatly for this to ever work. Makes sense though.

For uploads to correctly work, the temp folder and files folder much be created and setup correctly on the settings page. Maybe this needs to be made more clear somewhere in the docs perhaps or possibly appending the Temp patch to output a warning when trying to upload files when the settings page hasn't been viewed yet.

Going to set this back to commit since it is a different issue, thanks!

if i get it right, this was commited to core, if there's a later patch than the one commited this might be bullshit but it seems like this doesnt work:

1) create a new whatever node with upload support
2) attach and upload a file
3) now realize that you uploaded wrong file and try to delete it

doesnt work to delete it for me from that point. does for anyone other?

@saerdna: You mean delete it before submitting the node? Yes, that doesn't work. I think that's because the upload_nodeapi validate case doesn't check if 'remove' is checked and clear $_SESSION appropriately.

@Dries and seardna. Both your issues are known issues (but i cannot find them in the queue, grrr). I know that this makes it very hard to check this patch.

Anyway, I tested this patch with and without revisions now (before only without,hence I did not notice that bug) and all works as expected. At least no new bugs are introduced, but it does fix the bug where uplaods are completely borken.

I have tested patch 27. It works well for me excepted that the initial value of "List" is not respected.

ie: After attaching a file, it appears in the File attachments list with the "List" checkbox unchecked. But then, if I click submit, the file is listed at the bottom of the page. Editing the page shows the file as listed and if I change it back to not listed, that time it will not appear in the attachments list.

My problems aren't caused by a misconfiguration of mime_magic. file_check_upload() is being called twice, and the second time around the file has been processed and deleted from /tmp so mime_content_type() throws an error.

The scenario:
Safari, Javascript disabled. Edit a node. Select a file. Click Attach.
or
Firefox, JS enabled. Edit a node. Select a file. (don't click Attach.) Click Submit.

And the error reported:

warning: mime_magic: invalid mode 026763477601. in includes/file.inc on line 150.

Here are the backtraces for the two calls to file_check_upload:

file_check_upload("upload"), file: modules/upload.module line 208
upload_nodeapi(Object:stdClass, "validate", null), file: modules/upload.module line 166
upload_form_alter("page_node_form", Array[31]), file: includes/form.inc line  94
drupal_get_form("page_node_form", Array[31], "node_form"), file: modules/node.module line 1677
node_form(Object:stdClass), file: modules/node.module line 2000
node_page(), file: unknown line  0
call_user_func_array("node_page", Array[0]), file: includes/menu.inc line 355
menu_execute_active_handler(), file: index.php line  15

file_check_upload("upload"), file: modules/upload.module line 208
upload_nodeapi(Object:stdClass, "validate", null, null), file: modules/node.module line 316
node_invoke_nodeapi(Object:stdClass, "validate"), file: modules/node.module line 1555
node_validate(Array[31]), file: modules/node.module line 1572
node_form_validate("page_node_form", Array[31], Array[32]), file: unknown line  0
call_user_func_array("node_form_validate", Array[3]), file: includes/form.inc line 156
_form_validate(Array[32], "page_node_form"), file: includes/form.inc line 125
drupal_validate_form("page_node_form", Array[32], "node_form"), file: includes/form.inc line 100
drupal_get_form("page_node_form", Array[32], "node_form"), file: modules/node.module line 1677
node_form(Object:stdClass), file: modules/node.module line 2000
node_page(), file: unknown line  0
call_user_func_array("node_page", Array[0]), file: includes/menu.inc line 355
menu_execute_active_handler(), file: index.php line  15

if its a drupal-issue that files not can be removed before submit of node i think the delete-box should be removed from that view. but hmm seriously. imagine the scenario when you realized you uploaded the wrong file, would be handy if you could remove that file before submit of node.

I agree. If you have added a bunch of files in the current editing session and want to remove one before submitting the node, the delete checkbox should trigger the deletion of the tmp file and the record from the session variable.

This is different to the behaviour that was discussed earlier in the thread. On preview, the delete checkbox should not trigger the deletion of a file that was previously uploaded and is stored in the database. That should only happen on Submit.

While I agree with said comments, I believe these are outside the scope of this patch. This patch was merely to restore upload functionality to work with the 4.7 and revisions and so forth. All of these other issues have always existed and I think perhaps we should create a new issue that discusesses these further with another patch.

I would say that this patch still needs to go in to restore uploading functionality and then after we can come up with a patch that *improves* uploading to fix the above issues. Otherwise, this patch will neve go in and needs to go in now, thanks!

This would be the patch in comment #49.

The patch was committed.

Just to clarify, here are the issues that have been brought up since the patch was committed:

1) You can't remove an upload that you've attached since the last time you submitted (described in comment #54, #56, #61, and #62.

Reproduction:
a) Edit a node
b) Select a file and upload it using Attach
c) Select the Remove checkbox for that file and click Submit

Expected result:
a) The uploaded file is not attached to the node

Actual result:
a) The uploaded file is attached to the node

I believe this is caused because the validate case in upload_nodeapi doesn't check if removed is set

2) Richard Archer is getting mime_magic errors (comment #47, #48 and #60)

I'm unable to reproduce this using FF 1.5 or Opera 9.0P1+.

3) Similar to #1, the initial value of the list checkbox is not respected (comment #59).

Reproduction:
a) Edit a node
b) Select a file and upload it using Attach
c) Uncheck the List checkbox for that file and click Submit

Expected result:
a) The uploaded file is not listed when viewing the node

Actual result:
a) The uploaded file is listed when viewing the node

The cause of this is probably the same as in #1.

4) Dries is getting an htmlspecialchars() error when uploading. He's also getting a warning that the file was not uploaded. He did not set proper directory permissions, so that may be the cause (comment #52 and #53).

playnig devils advocate:
I think upload module has too many issues to pbe patched and patched and patched to fix all thise minor issues. Can we not rather move all this out of croe for a while (betteruplaod?) and fix up the whole module for once and for all?

Upload.module doesn't have any more issues than any other core module. The issues I mentioned in my last comment can probably all be fixed fairly easily. I see no reason why we should remove upload.module from core, especially at a time when so many people are calling for extended file attachment support.

*move, not *remove*. as in: put upload module (the whole file handling stuff in fact) in a separate repository hack on it with a load of people and then present a patch that fixes the ulaod module for once and for alll.

Your list grows much bigger if you take some time to hunt drupal.org for issues:
tmp directories, remote storage, IIS issues, mime problems, memory issues, odd and hard to grok interface, limited featuers, hardcoded featuers, bad theme support, no possibility to add files to anything else then nodes etcetcetc. I could go on for ages, if you'd let me :)

We have to face the fact that
* PHP s#$ks for file handling
* Every platform environment is so completely different that they need a separate set of code to fix all issues (approx 20% of the code in file.inc is to fix IIS issues)
* A lot of the uplaod.module needs comlpete overhaul, if it were to meet all our needs. That is *not* adding features, but architectural changes.

I don't see why removing upload.module from core will help fix these issues any faster. I also don't see what this discussion has to do with this issue. Removing a module from core doesn't fix the bugs mentioned here.

Uploads are broken for me in the 4.7 beta, and they were broken in CVS when I tried it a few weeks ago. I don't think this is a permission problem, as I've tested uploads with 4.6 on the same server and they work.

If I attach a file, it is uploaded into the server's /tmp but isn't being moved into the "files" directory. No attachments are shown when you view the node, but if I re-edit the node and click the "attach" button I can see the files I've tried to upload in the list that appears.

I've also seen the mime_magic error mentioned above. AFAIK, this module is correctly configured in Apache.

re: my above post - this happens in both Firefox and Konq, so I don't think it's related to the fancy-pants javascript wizardry. (which I could live without, personally, if it means I can upload files again!)

4.7 beta was released before this patch landed.

This seems to be fixed in CVS can this issue be set to fixed?

The problems mentioned in comment #65 need to be addressed in separate issue report.

Was a new issue created that addresses comment #65? Link?

I did not file an issue. There may be existing issues.