Incorrect preview permalink when attaching files

this is a known issue, and is caused by a limitation in drupal core. until we can set custom upload paths in some sensible way, we have to use a hack which corrects the path just before it's saved to the database -- so it looks like we'll have to just put up w/ this inconsistency until core enables us to fix it.

feel free to investigate further and post a patch if you do come up w/ a workable solution -- i had no such luck.

Hmm...looks like this is no longer the case in D6, for example, file field in Drupal 6 does allow you to set custom paths, and shows the correct path to the user after uploading the file.

@Steven Jones: if you can figure out how to do it, feel free to post that information. i'm not seeing how this is possible given the way upload module does it:

  // Save new file uploads.
  if (user_access('upload files') && ($file = file_save_upload('upload', $validators, file_directory_path()))) {

file_save_upload uses the value from file_directory_path, which gets its value from the 'file_directory_path' config variable -- so the only option i see there would be somehow temporarily changing the value of the config variable during the page execution, which seems a bit dangerous. ;)

Changing priority, as (according to jhodgdon), this blocks #528682: Allow inline images to be posted to Drupal.org project pages, docs pages, and comments without any special permissions

jhodgdon suggests using the Insert module for image file attachments.

Okay, here's a really lame, but potentially working, possible plan of attack:

Form alter after upload module, and swap out it's submit handler that it adds to the form here:
http://drupalcode.org/project/drupal.git/blob/refs/heads/6.x:/modules/up...

We can replace this with our own so that we can call a modified version of upload_node_form_submit:
http://drupalcode.org/project/drupal.git/blob/refs/heads/6.x:/modules/up...

In here we can just change the directory the files are going to be saved to for project issues.

As I said, this is a bit lame, and is basically the 'copy an entire function from core and change one line' approach, but there's a reason that people use file field and CCK: flexibility.

Added doxygen

This isn't sufficient, because the file isn't actually at the /issues/ path until the comment is saved, but you've made it look like it is as soon as uploaded, so when previewing the comment the image URL to the image is incorrect, leading to a broken image. This is then of course fine when you save the comment, but anyone who previews comments before saving is going to think they're doing something wrong.

We can fix that (easily) in the local image filter by checking one dir up if the file is not found. I won't reproduce that in this module.

Note that should not stop this from being committed -- it's broken as it stands it will be less broken if it gets in.

No, it's broken both ways! Sure, you can fix it in another module, but what about sites that use project* modules? Will they need a hacky fix in some input format too?

We're not addressing the actual issue of project module just moving the files on save here, I think the patch is taking the wrong approach.

Oh you mean I should move them on upload? I presume I can do that yes, stay tuned.

Yay, thanks! Mostly looking good and working well. The only problems I could find when reviewing the patch and testing locally:

A) Typo:

+        $ccomment_upload_storage[$fid]['filepath'] = $file->filepath;

it's $comment_upload_storage, not $ccoment. Fixing that locally seems to help some of the bugs I was seeing in edge cases. ;)

B) Probably I don't understand how file attachments work well enough in general, but it seems there's no longer any garbage collection if you attach a file to a comment and then bail on posting the comment. Maybe that never used to work, but I wanted to ask. Probably we don't care. I'm happy to commit this anyway, but I'm curious if/how this is supposed to work.

C) If you select a file and press "preview" instead of "attach", the little "Attachments" table in the preview still has the wrong link (even though the description on the form element itself has the right path). If you preview again it's right. Or, if you first "attach" then preview, it works. Probably not worth holding this issue up for this one, since it's definitely an edge case. But, if it's easy to fix, yay.

Thanks, chx!!

Cheers,
-Derek

I'm not sure this code handles new nodes does it?

Thanks for fixing typo.

Garbage collection never worked. it ran a file_save_upload the minute you pressed upload with JS. Or if it worked i want to know how!

Little table? I saw no table. Also without render API not 100% sure how easy to fix that one is. I would rather go with this as it is and fix whatever left if we can.

No, nodes are not handled.

As much as I deeply appreciate chx's time trying to get this to work, I'm very afraid to commit this patch. Even after talking it through on skype, neither of us actually completely understand why it works. ;)

I think a vastly better solution to this bug is:

#1282836: Stop trying to move issue attachments to a subdirectory of files

Let's see what the infra team has to say about that. If they're okay ripping out the moving, I think we should just kill this "feature" entirely.

Thanks again to chx for trying to make this work, but I think it's better to remove this feature from D6 (over at #1282836: Stop trying to move issue attachments to a subdirectory of files) than to keep trying to pile hack upon hack.