If the files directory is not writable, the color module, the proposed CSS preprocessor, etc. don't work. This suggests that information on whether the files directory is writable should be included on the status page (at least as a warning).

CommentFileSizeAuthor
#15 file_system_settings_0.patch2.02 KBChrisKennedy
#13 file_system_settings.patch2.02 KBChrisKennedy
#11 file_system_check.patch2.02 KBChrisKennedy
#9 patch_1631.81 KBmoshe weitzman
#7 patch_162492 bytesmoshe weitzman
#6 files_status.patch1.96 KBChrisKennedy
#5 files_status_5.diff1.86 KBpwolanin
#4 files_status_4.diff1.83 KBpwolanin
#1 files_status_1.diff1.62 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new files_status_1.diff1.62 KB

patch attached- makes this a warning, reports on status of public vs. private downloads

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Needs work

Great idea, this will help a lot of people out with the initial install. The patch appears to work correctly, although there is a typo in the error message: "You may need to change to set the correct directory"...

What do you think about providing better feedback by more thorough testing? It wouldn't be a difficult to also check for file_exists() and/or is_dir() to provide more specific error messages.

pwolanin’s picture

pwolanin commented

Looks like is_dir() is the better bet.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new files_status_4.diff1.83 KB

Ok, improved the descriptions, fixed typ0. Grabbed the same t() strings used in file.inc for the directory being not writable/not existing.

pwolanin’s picture

pwolanin commented
StatusFileSize
new files_status_5.diff1.86 KB

Should have looked more closely at file.inc. Try this patch instead- just to be extra sure there should be separate checks for is_writable and is_dir. Otherwise a writable file will pass in place of a directory.

ChrisKennedy’s picture

ChrisKennedy commented
StatusFileSize
new files_status.patch1.96 KB

This patch tweaks the code formatting and message strings. Otherwise it looks good.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new patch_162492 bytes

nice work all. i just changed 'file system' to 'files directory' because i don't like drupal telling me that the 'file system' is writable. thats a bit broad.

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Needs work

@moshe- seems like the wrong patch file...

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new patch_1631.81 KB

oops. here it is

dries’s picture

dries commented

Maybe we should change all instance of 'file system' to 'files' or 'files directory' then? (Not sure we should but figured we might ponder about it some more.)

ChrisKennedy’s picture

ChrisKennedy commented
StatusFileSize
new file_system_check.patch2.02 KB

From Drupal's perspective, "file system" consists of three dimensions (see admin/settings/file-system):

1. The "files" system path.
2. The temporary directory path.
3. The download method (public/private).

This patch currently checks items #1 and #3, so naming it "file system" is probably more accurate than "files directory" if we want to stick to the current terminology. If we want to globally change "file system" to something else that will be a lot more work. We could also address moshe's concern by clarifying that "writable" is referring to the files directory and not the server's filesystem, which is what I do in the attached patch.

pwolanin’s picture

pwolanin commented

Yes, that seems more specific and clear.

ChrisKennedy’s picture

ChrisKennedy commented
StatusFileSize
new file_system_settings.patch2.02 KB

Updated link text based on Dries' comment on http://drupal.org/node/102534

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Needs work

I think this needs to be an error, not a warning. That way, it shows up on /admin. If the files dir is broken, lots of things will be broken.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs work » Needs review
StatusFileSize
new file_system_settings_0.patch2.02 KB

Sounds good.

pwolanin’s picture

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

I think it's RTBC again- code works, patch applies cleanly.

Steven’s picture

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

The usage of t() in this patch is bad. Instead of doing t("Visit the %page", array("%page" => l(t('page')...))), you should do t("Visit the page", ...). That way, you keep the whole phrase together in one string.

I edited it, and also simplified the values. When nothing is wrong, we should be concise and informative. Only when something's bad, we should tell the user what to do in phrases.

Committed to HEAD. Thanks.

Anonymous’s picture

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