Upload.module does not generate an error if "files" directory does not exist, silent failure [#98391]

Related to http://drupal.org/node/73891, we need to not assume that the "files" directory has been created, attempt to create it with a file_check_directory() call, and generate an error if it can't be created.

Do a clean install of Drupal, enable upload module for pages, then create a new node with an uploaded file attachment (do not create a files directory or access the "File system" settings page yet). The file will appear to have uploaded, but when you click on the link a Page Not Found error is displayed. Why? Because you didn't go to http://example.com/admin/settings/file-system first to create your "files" directory.

No watchdog or error message is generate by upload.module either, it just silently fails. I've submitted a seperate issue to upload.module as this one should mainly deal with where we should be attempting to create the "files" directory. It could result that the answer is "won't fix" and we'll just include a file_check_directory() in upload.module somewhere so the the first upload creates the "files" directory like the "File system" settings page.

Comment	File	Size	Author
#14	upload_20.patch	1.62 KB	RobRoy
#14
#10	upload_19.patch	1.48 KB	RobRoy
#10
#9	98391.patch_2.txt	1.41 KB	dopry
#8	98391.patch_1.txt	1.41 KB	dopry
#6	98391.patch_0.txt	1.27 KB	dopry
#5	98391.patch.txt	1.27 KB	dopry
#2	upload_error	819 bytes	chx

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

cog.rusty CreditAttribution: cog.rusty commented 18 November 2006 at 22:00

I would suggest something mild, a refusal to upload until the user comes back after setting up the file system at the location the user wants and with the download method the user wants. Also, nothing disruptive which would make the user lose text at that time.

Comment #2

chx CreditAttribution: chx commented 20 November 2006 at 00:24

Status:

Active

» Needs review

File	Size
upload_error	819 bytes

wowsie, users want *more* error messages. Ask and you shall be given :)

Comment #3

RobRoy CreditAttribution: RobRoy commented 20 November 2006 at 01:00

Status:

Needs review

» Reviewed & tested by the community

Nice. This patch will attempt to create the default files directory if a user tries to upload before hitting the "File system" settings page.

Drawbacks
If for some reason the default files directory cannot be created or made writable, this patch will prevent a user from creating any node that has File attachments enabled regardless of whether or not the user is trying to actually upload a file with that node. We could maybe include an extra info message to point the user to the "File system" settings page to try to set up there files directory correctly, but that is for another patch. For this edge case, the current patch works great.

Comment #4

drumm

he/him

NY, US

CreditAttribution: drumm commented 21 November 2006 at 20:57

Status:

Reviewed & tested by the community

» Needs work

this patch will prevent a user from creating any node that has File attachments enabled regardless of whether or not the user is trying to actually upload a file with that node.

I think we should add the disabled attribute the file upload field.

Comment #5

dopry CreditAttribution: dopry commented 22 November 2006 at 04:03

File	Size
98391.patch.txt	1.27 KB

Here is a different approach that isn't prone to the same problems and proactive prevents the problem by not displayin the new file attach form.

Comment #6

dopry CreditAttribution: dopry commented 22 November 2006 at 04:03

Status:

Needs work

» Needs review

File	Size
98391.patch_0.txt	1.27 KB

Here is a different approach that isn't prone to the same problems and proactive prevents the problem by not displayin the new file attach form.

Comment #7

RobRoy CreditAttribution: RobRoy commented 22 November 2006 at 04:53

Status:

Needs review

» Needs work

I think we should combine both chx and dopry's patches so that we try to create the directories and then if it turns out that %path is not writeable, show a message and hide the form.

Comment #8

dopry CreditAttribution: dopry commented 22 November 2006 at 06:36

Assigned:

Unassigned

» dopry

File	Size
98391.patch_1.txt	1.41 KB

Here's one that attempts to create the directory.
If that fails it generates a rather generic for end users to report to their admins, and adds a link to the file system configuration page for admins.

Comment #9

dopry CreditAttribution: dopry commented 22 November 2006 at 06:39

File	Size
98391.patch_2.txt	1.41 KB

and heres one that actually checks file_directory_temp instead of double checking file_directory_path.

Comment #10

RobRoy CreditAttribution: RobRoy commented 22 November 2006 at 07:00

File	Size
upload_19.patch	1.48 KB

Looks good. Made a few SUPER minor changes to spaces and the t() link string.

Comment #11

RobRoy CreditAttribution: RobRoy commented 22 November 2006 at 07:05

Status:

Needs work

» Reviewed & tested by the community

Marking this patch RTBC as I only changed some spacing and the help description. Thanks dopry!

Comment #12

Dries CreditAttribution: Dries commented 22 November 2006 at 08:58

Priority:	Critical	» Normal
Status:	Reviewed & tested by the community	» Needs work

The message 'the file system is not properly configured' is too technical (and not 100% correct). It doesn't provide a solution or hint either.

This isn't a critical bug; lowering the priority.

Comment #13

dopry CreditAttribution: dopry commented 22 November 2006 at 10:37

1) I believe the description is fairly accurate. If the paths weren't created and aren't writable then the file system is not properly configured. I'm unskilled at being any less technical.

2) for admins it provides a link to the admin/settings/filesystem page which will actually display the error at the top of the form in a vibrant red box.

Should I else the if (user_access()) to say contact a site administrator for non-admin users or what?