Files attached to issues lose their file names and extensions; they are renamed 'issues_i' where i is an integer.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

I committed a critical file api fix to core a couple days ago. This might be fixed.

pulsifer’s picture

Status: Active » Closed (fixed)

At Home » administer » settings » project, set "Issue directory" to "issues". Then create a folder called public_html/files/issues and set the permissions of both public_html/files and public_html/files/issues to 777. This will solve the problem.

Dries’s picture

Any way we can improve the error handling/reporting? It wouldn't hurt to make this is tad easier to spot/fix/report/debug. Usability, eh. :)

pulsifer’s picture

Priority: Critical » Normal
Status: Closed (fixed) » Active

Status changed to reflect Dries suggestion

pulsifer’s picture

I noticed that this problem is currently logged (via watchdog?):

file system 2006-03-03 09:24 The directory files/issues is not writable, ... test

nedjo’s picture

We have a test in project_settings():


  $project_directory = file_create_path(variable_get('project_directory_issues', 'issues'));
  if (!file_check_directory($project_directory)) {
    $error['project_directory_issues'] = theme('error', t('Directory does not exist, or is not writable.'));
  }

So users setting it up for the first time should get a message.

dww’s picture

Title: Issue attachments renamed 'issues_i' » module doesn't create "issue directory" automatically
Project: Project » Project issue tracking
Version: x.y.z » 4.7.x-1.x-dev

whatever subdirectory you select via [site]/admin/settings/project_issue as the "Issue directory" is not validated and created for you if it doesn't exist. this results in weird behavior if you forget to manually mkdir yourself (all issues get uploaded as "issue" and you start hitting the file renaming stuff instead of using the names of the files you're trying to attach). plus, i guess some sites don't have shell access, where creating this directory would be more of a pain in the ass. we should validate to make sure the name they choose is a writeable directory or doesn't exist at all. if it's not there, we should try to create it, and mark the field as an error if we fail.

i recently submitted this as http://drupal.org/node/79138, but that's duplicate. moving discussion here.

Heine’s picture

Version: 4.7.x-1.x-dev » 5.x-2.x-dev
FileSize
3.23 KB

Attached patch gives warnings and disables file uploads when the issues directory has not been created or is not writeable. Also improved directory creation after code in system module.

Heine’s picture

Status: Active » Needs review

Patch against HEAD btw.

dww’s picture

Status: Needs review » Fixed

reviewed, tested, and committed to HEAD. backported, tested, and committed to DRUPAL-4-7--2 and DRUPAL-4-7, too.

thanks!
-derek

Anonymous’s picture

Status: Fixed » Closed (fixed)
agentrickard’s picture

Status: Closed (fixed) » Needs review
FileSize
1.71 KB

Odd. The current dev version does not create this directory, since $mode is not set in file_check_directory() calls.

Noticed when getting errors in the drupal.org testing profile installation.

aclight’s picture

Status: Needs review » Needs work

Based on the message shown if the directory doesn't exist, it seems pretty clear to me that the intent was not that the directory would be created on any issue node, but only by submitting the project_issue_settings_form. I'm not sure if that's really a good idea, but assuming it is then the first hunk of your patch probably needs to be dropped.

The second hunk of your patch doesn't make any sense.

-  file_check_directory($directory, FILE_CREATE_DIRECTORY, $form_element['#parents'][0]);
+  file_check_directory($directory, FILE_CREATE_DIRECTORY, $form_element['#parents'][0], 1);

According to http://api.drupal.org/api/function/file_check_directory/5 file_check_directory() only accepts 3 parameters, and your patch uses 4 parameters. Also, it looks to me like $mode is set properly here, because it's set to FILE_CREATE_DIRECTORY which happens to be defined as 1 (see http://api.drupal.org/api/constant/FILE_CREATE_DIRECTORY/5).

So ultimately I think this patch is unnecessary, as the module is already doing what it's intended to do.

However I'll set this to CNW because it's possible that we do want the directory to be created (or attempted to be created) when the project_issue form is built. Perhaps dww can chime in here.

agentrickard’s picture

Agreed. I think the first instance should be $mode = 1. The second is just a mistake.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
997 bytes

Corrected version of the patch (with part 2 removed).

dww’s picture

Yeah, I'm not sure we want this patch, since it means we might be trying to create the "issues" directory every time we build the issue form. But, I could go either way. It's good we check if it's there at all. And, I guess we have a default value for that setting, anyway, so it's not that bad to just create it using the default if the admin has never touched it.

I just don't know if it's best for usability to make the admin visit the settings page once and consider this default before anyone starts uploading issue attachments, or to just try to let the system automatically use the default immediately so users don't get bothered with a message about it. Once there are issue attachments using the default, if the admin wants to change it, it's a pain in their ass. However, I bet changing this default is only very rarely going to be needed.

So, +0. ;) I'll probably commit it, anyway, but I'd like to hear a little more feedback on my indecision, first...

Thanks,
-Derek

aclight’s picture

Status: Needs review » Needs work

I am also +0 on this. One consideration is how this change will affect things in D6. I know that file stuff changed quite a bit, but it looks like the file_check_directory() function didn't, so probably it won't matter.

If this is committed can we use the FILE_CREATE_DIRECTORY constant instead of 1 however. I'm setting to CNW just for that reason.

dww’s picture

I think the file changes from D5 to D6 are entirely independent of this patch.

Yeah, using the constant is better. @Ken: don't feel you need to reroll the patch just for that -- I can do that as I commit. We just need justification that we want this at all. ;)

Thanks.