file_move() not returning appropriate destination filepath?

I'm assuming this is a comment upload bug (until someone debugs this and can point definitively to a d.o configuration problem). This seems to be a new bug on d.o after the D6 upgrade. Giving this a slightly more precise title, since when looking at the previous title, I thought "that's by design".

ugh. it's not a bug in comment_upload i don't think, but probably in our rewrite code for files -> issues/files:

mysql> select * from files where fid in (3237904,3237910);
+---------+-------+---------------------------------+---------------+-------------+----------+--------+------------+
| fid     | uid   | filename                        | filepath      | filemime    | filesize | status | timestamp  |
+---------+-------+---------------------------------+---------------+-------------+----------+--------+------------+
| 3237904 | 61837 | password-strength-meter-2.patch | files/issues/ | text/x-diff |     5014 |      1 | 1235882010 | 
| 3237910 | 61837 | password-strength-meter-2.png   | files/issues/ | image/png   |    30198 |      1 | 1235882041 | 
+---------+-------+---------------------------------+---------------+-------------+----------+--------+------------+

filepath should include the filename, not just files/issues/

digging further, this only seems to be happening occasionally:

mysql> select fid, uid, filename, filepath, status, timestamp from files where filepath = 'files/issues/' and filesize <> 0;
+---------+--------+----------------------------------------+---------------+--------+------------+
| fid     | uid    | filename                               | filepath      | status | timestamp  |
+---------+--------+----------------------------------------+---------------+--------+------------+
| 3236338 | 365807 | addtoany-5.x-2.1.diff                  | files/issues/ |      1 | 1235562358 | 
| 3236340 | 365807 | addtoany-5.x-2.1-fixed.tar_.gz         | files/issues/ |      1 | 1235562374 | 
| 3237368 |   1631 | core_commands.drush_.inc_.364128.patch | files/issues/ |      1 | 1235759791 | 
| 3237380 |  93429 | perms_fieldsets.patch                  | files/issues/ |      1 | 1235762063 | 
| 3237534 | 443602 | CSS_tutorial.zip                       | files/issues/ |      0 | 1235782930 | 
| 3237904 |  61837 | password-strength-meter-2.patch        | files/issues/ |      1 | 1235882010 | 
| 3237910 |  61837 | password-strength-meter-2.png          | files/issues/ |      1 | 1235882041 | 
+---------+--------+----------------------------------------+---------------+--------+------------+

here's the code that does the rewrite of the filepath in project_issue.module:

/**
 * Rewrites the file information to move files to the issues directory.
 *
 * @param $files
 *   An array of file objects, keyed by file ID.
 */
function project_issue_rewrite_issue_filepath($files) {
  if ($issue_dir = variable_get('project_directory_issues', 'issues') ) {
    foreach ($files as $key => $file) {
      $file = (object) $file;
      $old_path = $file->filepath;
      $final_dir = file_directory_path() .'/'. $issue_dir;
      $move_path = $old_path;
      file_move($move_path, $final_dir .'/'. basename($file->filepath));
      $new_basename = basename($move_path);
      db_query("UPDATE {files} SET filepath = '%s' WHERE fid = %d", $final_dir .'/'. $new_basename, $file->fid);
    }
  }
}

couple of things to note:

• the file is moved successfully, ie it's where it's supposed to be, and the original file is gone.
• $new_basename appears to be empty given the data result we get in {files}.filepath -- this to me either suggests a bug in the file_move() workflow (as it's supposed to return the path the file was moved to), or some weirdness in the way basename() works.

i'm inclined to think the issue is with the file_move() code, so moving to the appropriate queue.

I confirm this bug on D6.10 & Project 6.x-1.x-dev & Project issue tracking 6.x-1.x-dev

I checked the db table and apparently, it does not occur all the time because only 2 out of 50 attached files had the missing file name, i.e. sites/mydomain.com/files/issues

I haven't looked into the code yet, nor did I try to reproduce it yet. To be continued.

Almost 3 years after date, I reproduced this consistently.

Upload a file using comment_upload to a project_issue node
AND
the project_directory_issues textfield on admin/project/project-issue-settings has a value like "issues"
Description of that textfield says in my case: "Subdirectory in the directory 'sites/mysite/files/' where attachments to issues will be stored."
AND
you choose to preview the comment
THEN
the file will be moved

When you then either hit submit or another preview
the resulting filepath in the files table will be: sites/mysite/files/issues/

The immediate solution:
Set project_directory_issues on admin/project/project-issue-settings to empty (no value) and save it.

Long term solution:
Change the code so the file move is not done when doing a preview.

Note:
In #3, one mentions that the problem occurs occasionally. Indeed, only when the submitter has previewed the comment with an uploaded file to it (and when project_directory_issues is not null obviously)

Hope this helps to crush this issue ;)

The call to change the path is done in project_issue_form_comment_validate:

  // Adjust new file attachments to go to the issues directory.
  // We have to do this during validate, otherwise we might miss
  // adjusting the filename before comment upload saves it (module weighting)
  // TODO: is this still true?
  project_issue_change_comment_upload_path($values);

So I assume we should add a conditional check there: only execute this when comment is actually being submitted, not when being previewed.

Assigning it to the right project

This fixes it

  if ($values['op'] == t('Save')) {
    project_issue_change_comment_upload_path($values);
  }

Phew, after 3 years ;)