now that ifac has landed, we have a bit of a weird situation with file attachments for issues: the initial issue attachments and the comment attachments are handled totally differently.
i much prefer the way the comment attachments are handled (basically using the native upload functionality, which allows multiple attachments!), as well as how they are rendered in a table below the comment itself. this seems to follow a more logical flow -- you read the issue details, then download the attachment.
also, i think it's confusing to embed the original issue attachment in the main metadata -- the problem here is that often that attachment is outdated as soon as someone uploads another, and yet it's advertised as 'most current' because of it's position in the summary metadata table.
given all of the above, i think the best course of action is to rip out the file attachment functionality of project issue altogether, and simply use upload module for this. examining the capabilities of upload module (esp. on drupal.org), this appears very doable to me. uploads can be enabled per-type, and the default list of allowed file extensions is reasonable (although i'd love to see .patch on there).
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | issue_files_2.patch | 24.13 KB | hunmonk |
| #4 | issue_files_1.patch | 24.04 KB | hunmonk |
| #2 | issue_files_0.patch | 22.16 KB | hunmonk |
| #1 | issue_files.patch | 21.4 KB | hunmonk |
Comments
Comment #1
hunmonk commentedhere's the initial patch. i've tested the upgrade path on postgres, and it works beautifully. also tested creating new issues, editing issues, adding and deleting issue attachments -- all seems to work perfectly.
couple of things:
upping this to critical, as it's been approved for use on d.o
Comment #2
hunmonk commentedok, file location problem solved... :)
i lifted a clever little hack from crell's Upload path module. it's not robust enough to handle per node type custom paths, so i think this is the best solution for now.
afaik only mail.inc remains.
Comment #3
dwwQuick visual inspection of the patch looks fantastic. 234 lines that start with '-' and only 125 that start with '+', it improves the functionality (e.g. attaching multiple files to the initial post), and simplifies the UI. ;) Now that's a good patch. Still needs testing, and whatever changes to mail handling, but this is looking extremely promising. Thanks!
Comment #4
hunmonk commentedwe still needed to adjust the filepath for files added in followups, so updated patch attached. i had a look at heine's patch for comment upload, but sadly, it works the same way that upload path does -- globally. that's not going to work for us here, so i've again rolled a mini-implementation specific to project issue. if/when the path handling for file uploads gets more robust, we can pull this out.
also note that i had to use a different, slightly uglier approach than issue files -- if project issue and comment upload aren't weighted properly, then we would miss the opportunity to rewrite the path during insert/update, so the rewrite is handled in the validation step.
also, i would very much like to handle the mail.inc changes in another patch. this patch is getting big enough as it is. that's how i handled the original ifac conversion, and it was a good division of tasks.
Comment #5
dwwre: mail.inc in another issue: no problem.
re: upload path robustness woes: sure, that's totally reasonable. if the state of the art at this time isn't flexible enough to cleanly handle it, we hack in a work-around for now, and someday when the file API situation improves, we change it. the drop is always moving, after all...
re: module weights and hacks: there's nothing wrong with setting module weights appropriately to make something work how we expect. i know the whole module weight system isn't ideal yet, but it exists, and this is exactly the sort of thing it's for. diff.module sets its own weight so it can clobber the menu path at node/N/revisions, for example. so, if it's important that project_issue is heavier than comment and comment_upload, so be it. we already set pi's weight to 2 during hook_install() to deal with some weight-specific ordering problems before (basically, all the menu paths get screwed up unless project is lighter than project_issue and project_release). if that's not heavy enough, we can just change that and add a new db update to update it. in general, i'd rather do that than use ugly FAPI tricks. we could even add a hook_requirements check for project_issue to make sure the weights are correct relative to all the other modules it needs to outweigh. ;)
so, i'm setting to CNW so that you either: a) followup here to say "the FAPI hacks aren't so bad, and i'd rather not rely on the weights because ...., so please just review #4 closely" or b) post a new patch that simplifies the FAPI-ness and relies on the fact that we're already setting weight. adding the requirements check can be a different patch in a different issue, if we want to further rely on that. either way, once you reply again, i'll give this a close review and testing so we can get it in.
thanks!
-derek
Comment #6
hunmonk commentedit's not project issue that needs to move, but comment_upload. it's default weight is 0, but it needs to be _heavier_ than project_issue (b/c it must save after project_issue modifies). i really don't want to mess w/ the weight of another module.
and the fapi hacks aren't that bad at all. it's simply a form_set_value() in validate.
Comment #7
hunmonk commentedsame patch, i just added a defensive programming peice to project_issue_change_comment_upload_path() -- since it's run in validate, there's an outside chance that it could get called twice, so i put a static switch in there to make sure the rewrite is only performed once.
really just needs mysql upgrade testing, and maybe a quick couple of UI tests. i've tested the functionality pretty thoroughly.
Comment #8
dww- Patch still looks good
- Tested the DB updates on mysql and they all seemed to work fine
- No time now for a thorough review of project_issue to see if there's any lingering dead code, but we can always clean up later if we happen to find anything.
- Played with the new attach UI -- my only gripe is that we should expand the "File attachments" fieldset by default on issue and comment forms. But that can be another issue/patch. Also found some weirdness with the JS on safari, but that's a core bug, nothing we need to worry about here.
Comment #9
hunmonk commentedcommitted to HEAD. mail.inc conversion is being handled here: http://drupal.org/node/185208
Comment #10
dwwExpanding the "File attachments" fieldset now has an issue (and patch) over at http://drupal.org/node/185210
Comment #11
(not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.