Implement magic for the table of attachments on issues

Wait, you mean the UI at http://git7site.devdrupal.org/node/1706064 isn't already ideal? ;)

Uhh, yeah... wow. This is totally unworkable as-is. http://git7site.devdrupal.org/node/1706064/edit is pretty harsh, too. In some ways, that's a harder problem to solve. When viewing the issue we could play tricks with hiding files along the lines of #1245508: Issue summaries: Allow outdated attached files to be hidden/replaced/flagged.. But on the edit UI, we need some way to hide irrelevant stuff, and I'm not sure what that would be.

I think the community is going to be in serious revolt if we allow users to delete each other's files. So, we need to form_alter() the remove links out of the picture (at least for files that already exist -- we could still let you delete a file you just uploaded before you save it). But certainly "remove the old crap when you upload better stuff" isn't a solution for either the view nor edit pages.

I'm not even 100% sure what to do here, so I'm going to solicit some feedback from IRC and folks at BADCamp over the next few days and see if we can come up with a good solution that's not too terrible to implement.

I had a long and fruitful IRC chat with Fabianx about this. I'm going to try to summarize.

One thing we pretty clearly need is to resurrect the "Show" checkbox on files. We need to let people show/hide files. When viewing the issue, we only show the files that are configured to show. When editing the issue, we're going to want something like a fieldset for all the visible files, and a collapsed fieldset with all the hidden files (just in case you need to show something that's been hidden).

Furthermore, we clearly need to *not* use the default file upload widget for this edit form. ;) We'd have to jump through extra hoops to disable the remove buttons, hide all the confusing weight stuff, blah blah. Screw that. Let's just use a different widget.

Now, there are a couple of different underlying technical approaches we could use.

A) Continue to use a regular core file field directly, with out own widget (along the lines I described above) with a show/hide checkbox, filename, and the default file upload widget with the AJAX magic. The checkbox can be stored with the field (there's already a 'field_issue_files_display' column in the {field_data_field_issue_files} table -- but core's default file widget no longer exposes that field in the UI). Fabianx assures me this actually wouldn't require much code. He also pointed to http://drupal.org/sandbox/aland/1552756 which might have some useful code we can harvest.

B) Do what we did in project_release and use a field_collection for files attached to issues. Then, each file can have additional metadata as full-blown fields (e.g. test-bot related stuff, a checkbox for "please don't test me", whatever). From IRC:

[9:32pm] Fabianx: dww: field_collection is a good choice, because:
[9:32pm] Fabianx: entities can be updated individually, without having to update the parent node.
[9:33pm] Fabianx: dww: You would just list: inactive: entity_id, checkbox.
[9:33pm] Fabianx: dww: active: entity_id, checkbox
[9:34pm] Fabianx: dww: And for newly uploaded files just: checkbox, description, file
[9:34pm] Fabianx: dww: Then on save: You copy all updates to the respective entities.
[9:34pm] Fabianx: dww: That makes the form to manage / upload really simple and the update logic easy ...
[9:35pm] Fabianx: dww: Its not a file_managed widget than, but just a custom form with a number of file_managed_widgets plus some additional data per row.
[9:36pm] Fabianx: dww: That can be as simple as:
[9:36pm] Fabianx: file_0, state_0, description_0
[9:36pm] dww: Fabianx: one thing that gets slightly more ugly is that it's potentially harder to have the auto-generated comment know about the field_collection entities that were added/updated with that revision...
[9:37pm] dww: It'd be interesting to see how people would actually use a description field on issue files...
[9:37pm] Fabianx: dww: In that case you could still update the field_collection entries instead before submitting. Just need to get the order right.
[9:37pm] Fabianx: dww: :-D
[9:37pm] Fabianx: dww: But maybe just changing the comment logic might be easier.
[9:38pm] Fabianx: dww: As all data is available ...
[9:38pm] Fabianx: dww: And a simple:
[9:38pm] Fabianx: x = entity_load('fc_item', $id);
[9:38pm] Fabianx: should not hurt to get the filename to update.

I'm not convinced field_collection is worth the trouble (especially since it'd mean we'd have to change the data migration code). But, I just spoke to jthorston of the testbot team and he thought having a real field_collection entity for every file attached to an issue would actually be a good thing for them. Apparently, in D6, a lot of the interesting testbot-related stuff (status, link to the results, etc) *only* lives as formatted HTML written directly to the comment. Seems a lot cleaner to actually have real storage for this sort of thing.

However, project_issue itself doesn't care about the testbot per se. So if we provided a default field_collection for this, it'd just have some simple metadata fields, and Something(tm) would have to extend the field collection.

As I write up this summary, I'm leaning towards A. Using a field_collection is potentially cool, but between changing the data migration and all the hoo-ha for extending the field collection configuration for the test bot's benefit, etc, etc, it's just a lot more work (especially since we'd *still* have to have a custom widget for this approach, too). And we don't have time for cool solutions that require extra work. I think we just need to implement the quickest/simplest path to victory here.

I need to go home and sleep, anyway, so we can leave this open for further input for the next 12 hours or so. But probably tomorrow late morning PDT I'll start implementing approach A unless something major shifts in here.

Thanks,
-Derek

Right, so there are 3 parts:

A) Custom field widget for the upload UI when creating/updating an issue. PIFT doesn't care about this at all.

B) Custom field formatter for the display UI when viewing an issue. PIFT wants to be able to inject its own stuff into this. Contrary to what some of us suspected during our conversation about this, the Field Formatter API is *not* class based, so unless we go out of our way to OOify things, it's not going to be trivial for PIFT to extend the formatter we're going to write. Therefore, the good ways for PIFT to extend this would be:

• Maybe with enough theme template and preprocess magic PIFT could inject its own goodies
• We invoke some kind of alter hook
• PIFT uses the core content view altering hooks to alter the content on the way to the screen
• ?

C) Fixes to nodechanges for when it's rendering a comment that corresponds to a node revision that changed the attached files in some way. For now, it's just hard-coded to use theme('file_link'). Perhaps there's a way to have it re-use the field formatter?

These 3 parts could potentially be split into separate issues if we wanted, although in practice I suspect I'm just going to write all of it, anyway.

Okay, this issue is now strictly about the file field formatter to display the table of files at the top of an issue node. The other parts of this effort now live here:
#1833684: Need a file widget that gracefully handles lots of hidden files
#1760658: Come up with a formatter for file fields

Note that to actually get the formatter to tell us what we want, we desperately need a solution for #1549280: Provide an explicit relationship between patches and comments about patches.

Assuming that's done (and has its own time estimate), I'm guessing we're going to need 3 days in here to come up with a good solution that's easily extendible by PIFT, implement it, make sure our file field is configured properly to use it, etc, etc.

I still haven't really started it. Life got crazy again for the last couple of weeks, and I haven't been able to make much time for focused development. :( I'll spare you the personal details, but suffice it to say I haven't been in top form (and the holiday didn't help, either). I'll see if I can push this forward a bit over the weekend. Monday I'll be on a plane for a while, which is often a good place to write code. And the rest of next week during the day I should have much more time for coding. I'll definitely update here with progress as it happens.

Sorry,
-Derek

p.s. One bit of good news is that I found a place to live for the rest of December that will have reasonably good internet access, so I'm *not* completely disappearing after Dec 7th. ;) That doesn't mean I want to delay this, or downgrade the priority for getting it done, but I wanted to let y'all know that all hope isn't lost if there's still more to do after next week...

Excellent, thanks! I'll play with this later and give a real review, but a quick skim on my phone looks great.

Meanwhile, if I have time today on the plane to hack on this stuff, I'll focus on #1833684: Need a file widget that gracefully handles lots of hidden files so we don't collide. ;)

Thanks again!
-Derek

I'm a bit worried about the usability of a scrollable table, especially on mobile devices. Also, keep in mind we definitely want to distinguish visible vs. "hidden" files, so even if we had a scrollable table to gracefully handle the case where there are a large # of visible files, it'd just be showing the visibile files, and we'd *still* need a way to show *all* the files, including the hidden ones.

I'd be happy keeping this more simple for the first pass, and iterate once we're live and gathering real usability feedback. Of course, it'd be nice for it to be slick and perfect when we first deploy, but I don't believe we'll be able to do that in a vacuum, and I think there are a lot of things more urgent than polishing this particular issue to perfection. For example, it's not clear to me yet how PIFT is going to hook into this table to inject its stuff. We're also definitely going to need something better for #1760658: Come up with a formatter for file fields (e.g. something that shows in the auto-generated comment for a node revision which files you hid, made visible, and/or attached during your edit), and PIFT probably wants to hook into that, too. If you still have energy to spend on this stuff, I'd invite you to start looking into those things (especially the PIFT-injection aspect) before worrying about a scrollable table here.

Thanks!
-Derek

p.s. FYI: This is at the very top of my list to look at Monday morning when I'll have a real block of time to work on this stuff again.

I hadn't read the patches yet to find the alter hook, thanks for that. ;)

I don't think we can re-use the same formatter on the comments, since we really need the comments to show the diffs to this table that were generated by a given node revision. Even if it was easy to reuse some of this code to say "only show the files attached during a given node revision", we'd actually want it formatted differently, since we'd need to indicate those were the files added. And, as I said, we also need to be able to show files that had already been attached via another revision that were hidden in the current revision, and files that were attached and previously hidden that were made visible in the current revision. That's probably a discussion we should be having in #1760658: Come up with a formatter for file fields not here, but whatever. It's fine to have this sort of acting as the meta issue about this functionality.

Yes, I think a static cache for project_issue_get_file_cids() makes a ton of sense if that's something we're going to be calling multiple times on a single issue page. If it's a lot of work and queries to figure out what files were attached in which revisions/comments, we should save that and re-use it.

I'm also still slightly concerned about relying on comments with a nodechanges field on them to do all this mapping. Maybe that's really the best approach, but I don't think it's insane to have an issue files metadata table of our own to store things we care about directly. Maybe an extra table is unnecessary, but I think we should keep it as an option that might possibly be a cleaner approach. Once I carefully review the actual code here and have more familiarity with what you've done, I'll probably have something more concrete to say. But for now, I just wanted to invite you to ponder that as an option as you're working on this. While we're trying to use as many "off-the-shelf" parts for this functionality as possible, issues really have some very specific needs regarding files, and I don't think it'd be the end of the world to have some custom storage and code if we need it. This whole set of issues is about custom magic for files, anyway. ;)

Thanks again for all your work on this!
-Derek

p.s. And thanks for the latest patch. I'll take a look at both.

Cool, I'm glad you think it's easy to have a single formatter handle all cases. That's certainly better for code-reuse purposes. I also agree it'd be great to just save the cid along with the file, instead of trying to re-compute it (even just once) on every issue page load.

Re: cids + files -- don't forget about the case for this particular issue -- the table attached to the issue node itself. For that, we want "jump links" next to each file which bring you to the comment that describes the node revision where that particular file was attached. For that case, we don't have a single obvious cid. Maybe you're just saying that we don't have to worry about caching since we wouldn't be adding these links to the table for each comment, only once for the initial post.

If the UI doesn't get too confusing (which it probably would), maybe it'd be cool to have some way to also see any revisions/comments that changed the visibility of each file not just the revision it was first attached to the node. But that's definitely something we can add later, or never, so it shouldn't slow us down here.

I'm not sure if nodechanges currently tracks the show/hide bit on files. I kinda doubt it. If not, we should open a new issues in the nodechanges queue about adding that support (or just handle it as part of #1760658: Come up with a formatter for file fields, although it does seem fairly separate).

Thanks!
-Derek

Thanks again for all your work here. Some quick thoughts (again, based solely on your comments, not the patch):

Both 1) and 2) could be solved by keeping the hidden files in a separate table, which is collapsed by default, and there's a link (I'm not sure checkbox makes sense) to reveal the entire "Hidden/Obsolete files" div and the table it contains.

3) No, there's no "final" call on the UI yet. We just have to try to build something that Doesn't Suck(tm). ;) Once it's further along and we've done a reasonably good job, we can get some UI folks like bojhan and yoroy to take another look and give feedback. I've got a fair bit of UX experience by now, and drumm does, too. Technically, he's the "product owner" for Project* on d.o now, so once the two of us are happy, we'll be pretty set.

That said, I think we're going to want a full-width table for a couple of reasons:
- We're trying to pack a lot of into into this files table.
- The file attachments are some of the most important parts of an issue.
- Most (all?) of the target audiences that care about an issue care about the attached files: people looking for a patch to fix a problem they're having on their site, contributors/developers trying to improve a project, and project maintainers.
- We're trying to shift the usage of issues more in the direction of keeping the issue itself current and clear, instead of forcing everyone to always read the entire comment stream to know WTF is going on. So, we should make the relevant files visually prevalent at the top of the page, instead of hiding them in a collapsed block and reenforcing the idea that people have to scroll through the whole issue to find all the files (and then try to figure out which one(s) matter).

Also, the horizontal tabs thing (with commits vs. files) is only relevant once we actually *have* commits to display, but neither #493074: Back-link to the commit as a comment on the related issue. nor #443000: When viewing an issue, display a list of commits that reference that issue # seem to be moving, so I think that'll have to be done on another iteration. For now, let's assume we don't have any kind of commit stream. If/when we do, we can figure out how we want that to appear in the UI.

4) Out of scope. It was just an idea, but I'm retracting it since it's mostly not all that relevant, and we don't want to complicate the UI for everyone (nor the code) in service of an edge-case.

Re: re-using this formatter for nodechanges -- there's already a formatter that nodechanges uses for displaying its stuff. That's what #1760658: Come up with a formatter for file fields is about -- we want to extend/improve the file formatter it's using now to give it more of this goodness. Exactly how to best do that will probably make more sense once we actually start writing that code. If we have to refactor anything in here to be able to re-use more code, we'll have to cross that bridge when we get to it. One complication is that we don't want nodechanges to have a circular dependency on project_issue. So, it's not obvious where the code that was being re-used would live. But, let's not worry about that too much here, and just handle that over at #1760658.

Re: drupal_alter() -- yes, I think that's fine, although it's not clear to me exactly what's being altered. Are we passing around a nested array defining table headers and rows, letting people alter that, and then sending the final result off to theme('table')? I guess I should just RTFP. ;) But yeah, assuming we're altering some kind of structured array, and not just raw HTML, I think that's cool. There might be other alter hooks folks could use for this, but I doubt they'd be operating on the level of structured data, which is where we really want to allow the altering to be happening.

Re: 27 -- yes, the show/hide bit for a file is the core 'display' property on files. For whatever reason, the default file upload widget in D7 no longer presents this as a checkbox for the user (it was probably considered too confusing for most end-users and a D7UX "fix"), but the property is still handled by the core file API and stored with the file field. We're going to expose this again in the file uploader we're going to use for issue file fields, which is one of the main points of #1833684: Need a file widget that gracefully handles lots of hidden files. Nothing is currently being hidden because that widget doesn't yet exist, but it will soon, and then we can start playing with this functionality for real on various dev sites.

Thanks again! I'm going to actually dig into the patch now (yesterday was problematic for a few reasons, but I've got some time right now to push this forward).

Cheers,
-Derek

Started going through the code with a fine-toothed comb. Mostly, I liked what I saw. Here's the first round of feedback (and a patch that fixes most of it):

A) Lots of comments needed to be real sentences. Fixed.

B) The alter hook should use the 'project_issue' namespace. Fixed.

C) Missing a project_issue.api.php hunk to document hook_project_issue_files_summary_table_items_alter(). Mostly fixed. I see we're getting structured data, which is great. However, it's slightly weird that $items is the first argument, implying that's the meat being altered, and $header and $rows are just context, when in fact, $items is the context and $header and $rows are the meat. I know drupal_alter() isn't setup to cleanly let you alter 2 things (although the "context" is passed by reference). But, I almost think it'd be better to completely reverse the order of args so that you get $rows, $header, $items to make it more clear you really need to alter $rows (and perhaps $header) and that $items are just there for your context.

Furthermore, trying to document this alter hook made me realize some problems with how the data is setup and being passed. $rows aren't keyed by anything. So, say you wanted to iterate over $items, and inject another column for each file. There's no way to find the corresponding row based on any of the context in items. Even if you iterated over $rows so you were at least operating on the right row at each point, there's no way to find the right entry in the $items array to find the context for that row.

First I thought "okay, cool, I'll just key $rows by fid since that's guaranteed to be unique, and will be ignored by theme('table'). But then I realized that'd clobber the sorting done by project_issue_field_formatter_view_files_sort(). Which brought up a related question -- do we want to let people reorder this table via the alter hook? Maybe the solution is to key $rows by fid (so altering is easy and clean) and then just call project_issue_field_formatter_view_files_sort() after the alter hook returns. That way, the sorting settings are always being honored, regardless of what the alter hook is doing. However, perhaps people want to be able to re-sort in the alter hook. Ugh. I'm not 100% sure the best approach here, but I know it's going to be an ugly pain in the ass to use the alter hook given how the data is currently being passed. Thoughts?

D) 'allowhooks' seemed like a weird name for that setting. It's really about 'allow_alter', so that's what I changed it to. That said, why is this a setting at all? If there's an alter hook, why not just always invoke it? Why would anyone want to configure a field to display without the possibility of being altered? Partly fixed. ;)

E) That got me thinking -- wouldn't you want the $field and $instance as additional context for the alter hook? I've run into situations before where I really wanted that as context in some alter situation and didn't have it. Ditto $entity and $entity_type. So, I turned the final arg to the alter hook into a nested $context array with subarrays for $items, $field, $instance, $entity and $entity_type. Fixed.

F) Given what I wrote at #30, we probably want different behavior for the 'showhidden' setting. I guess that complicates the alter hook, too, since we'd want separate $rows arrays for the different resulting tables (and perhaps separate $header arrays, too). Not fixed -- punting for now until we get closer to a real plan for hidden.

G) It doesn't seem like project_issue_get_file_cids() needs to assume field_issue_files, does it? We could pass in the field name as an argument. Not fixed (since I'm not sure this is a good idea or not). ;)

H) Related to G -- project_issue_get_file_cids() is assuming a single 'project_issue' node type. We don't want that, since a site could potentially have many node types configured to be issues. See project_issue_issue_node_types(). If this function was getting passed a fully-loaded $entity/$node object, it wouldn't have to worry about this at all -- it could just figure out the right comment bundle name to use based on $node->type. Not fixed.

I) I'm getting a PHP notice on this line:

        foreach($change['old_value'] as $file) {

That's because $change['old_value'] can be NULL. For example, on the issue I'm testing, the original issue node didn't have any files attached. I edited the node to attach some files, which generated a comment showing the first files attached. When this code hits that comment, it's confused. Not fixed.

J) We should configure out default issue node type to make use of the new field formatter. Fixed.

So, here's my patch that includes all of your new code with A, B, E and J fully fixed, and C and D partially fixed. F, G, H and I all need work, as do parts of C and D. I'm going to stop here and get feedback from jthorson before proceeding. If you want to re-roll this, go ahead. I won't change this code again until you reply. If I have more time to hack on this stuff today, I'll get started at #1833684: Need a file widget that gracefully handles lots of hidden files.

Thanks!!!
-Derek

Great, sounds like we're on the same page on basically everything. ;)

C) I'm still slightly torn about this. Maybe add a default sort option (only for the formatter settings page, not exposed to the end user) called 'Computed order' or something, and if yes, don't sort after the alter hook returns? Just in case someone needs it. Perhaps call it 'Computed order (allow altering)'? Probably we can document it in the README and leave it shorter in the UI. By doing D, this is effectively no more code than the existing patch, we just need to move and repurpose a few lines and it's done. I believe that based on the current patch If this was selected, the default order before being altered would be fid ascending

F) I like changing the setting as you describe. However, instead of separate hook invocations, we could add another layer of nesting (keyed by 'visible' vs 'hidden') to the $rows and $headers arrays we pass to the alter hook. Hook implementations will have to know, but I think that's a reasonable API agreement to make in exchange for not doubling the # of hook invocations for this.

G) A separate module for this formatter shared by both project_issue and nodechanges would work. It seems like a bit of overkill in some ways, but it's probably for the best. As you said, the only part of this that has to know about nodechanges and comments could be altered into place,. It's a bit of a shame to deploy/maintain a separate module, but on its own it will probably find a wider audience than it will living inside project_issue.

The only change in the attached patch is a fix to another tiny code style bug I noticed: a missing space in foreach(. If/when you are able to start working on any fixes or refactoring, please assign yourself first so we don't duplicate effort here. I'll do the same.

Thanks again!
-Derek

Excellent! I cloned and took a quick peak at the sandbox. Mostly looks great, thanks! We seem to have x-posted, so it doesn't reflect some of my recent comments, but this is definitely most of the way there.

Does it make any sense to propose adding this to the file_formatters project? That's basically the only existing full project I found with a few quick searches. However, that project doesn't appear to be maintained (or widely used), so it's unclear what we'd gain. I think you should probably go ahead and promote this to a full project, since we're clearly going to need it for the d.o D7 upgrade.

The only other thing I found was Field formatter settings -- does a dependency on that help us (as the project page suggests it might)?

Thanks again!
-Derek

Great, thanks!

If I want to do some minor cleanup to the sandbox code, should I push changes directly or do you want to see patches, first?

Do you want to promote it to a full project?

Are any of the points I've already raised still to-be-implemented, or do you think your refactoring solved them all? I can obviously just figure it out myself with another detailed review of the code, but if you know now that there's unfinished work, it'd be nice to know. Also, if you're planning to do anything else with this, let me know.

Thanks!
-Derek

Excellent, thanks! I just pushed a few commits with some cleanups (including the ones from #33 and my prior patch), and opened some new issues in the queue for other ideas I had based on another pass through the code:

#1867094: Pass more context to hook_file_metadata_table_items_alter()
#1867098: Rename 'uid' to 'username'?
#1867100: How should sorting work for altered columns?

I also split F off as #1867102: See if we can avoid a whole separate hook invocation for the table for hidden files since this is starting to get confusing. ;)

I'm setting this issue back to active since it needs a whole different patch from #33 where project_issue implements the appropriate alter hooks. Some of the functions can be harvested obviously, but it seems more accurate to say we're back to the drawing board for project_issue now that file_metadata_table.module exists.

As always, if you're going to work on any of this, please assign yourself first.

Thanks!
-Derek

@webchick: If field_formatter_settings contrib works for now and we don't need the core backport patch, all the better. That'd be a cleaner thing to do with d.o's bzr tree then having to commit the core backport patch (which would have been the other option). I'll also make sure we've got latest -dev of file_metadata_table in bzr.

@jthorson: Can you confirm field_formatter_settings contrib is all we need for now? The patch seemed to work as expected with neither the core patch nor the contrib, but perhaps I just didn't try the thing these other pieces of code enable. Can you clarify what that is? ;)

Thanks much for the new patch. Sadly, I think you were right that hard-coding this as a dependency is a bad idea, especially in light of #1952792: Make the default project + project_issue install easy to use for non-software project management. So, I think #45 is better, and we should move parts of the interdiff with #46 to a patch for drupalorg where we're creating the d.o versions of these node types. We could either move this in as a new update_N() inside drupalorg_project/drupalorg_project.install or perhaps just add something to one of the existing updates that's customizing these node types for d.o. However, it doesn't look like we do any customization of the issue node type yet, so let's just create a new update for that.

However, when I'm trying this patch on a local test site I'm getting this when viewing issues:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')) ORDER BY comment.cid ASC' at line 2: SELECT DISTINCT field_data_field_issue_changes0.entity_type AS entity_type, field_data_field_issue_changes0.entity_id AS entity_id, field_data_field_issue_changes0.revision_id AS revision_id, field_data_field_issue_changes0.bundle AS bundle FROM {field_data_field_issue_changes} field_data_field_issue_changes0 INNER JOIN {comment} comment ON comment.cid = field_data_field_issue_changes0.entity_id WHERE (field_data_field_issue_changes0.field_issue_changes_field_name = :db_condition_placeholder_0) AND (field_data_field_issue_changes0.deleted = :db_condition_placeholder_1) AND (comment.nid = :db_condition_placeholder_2) AND (comment.status = :db_condition_placeholder_3) AND (field_data_field_issue_changes0.entity_type = :db_condition_placeholder_4) AND (field_data_field_issue_changes0.bundle = :db_condition_placeholder_5) AND (field_data_field_issue_changes0.entity_id IN ()) ORDER BY comment.cid ASC; Array ( [:db_condition_placeholder_0] => field_issue_files [:db_condition_placeholder_1] => 0 [:db_condition_placeholder_2] => 22 [:db_condition_placeholder_3] => 1 [:db_condition_placeholder_4] => comment [:db_condition_placeholder_5] => comment_node_project_issue ) in field_sql_storage_field_storage_query() (line 582 of /.../modules/field/modules/field_sql_storage/field_sql_storage.module).

Maybe that's because I have a bunch of files attached to the original issue and there's no corresponding comment at all? I'll keep investigating what's going on. Perhaps it's another subtle bug from using devel-generate'ed content. The Drupal equivalent of fuzz testing our code. ;) Yes, initial testing points to that: the formatter blows up if there are files attached to the original node revision and there's no comment at all, but I haven't looked for the actual bug yet. Probably easy to find once I look.

Meanwhile, are you available to work on the patch for drupalorg where we're creating our d.o D7 node types and just make this a dependency there instead? If not, no worries, I can get to it. Just let me know.

Thanks!
-Derek

Great, thanks! Much better. That was basically working on all issues, regardless of where the files were attached, so yay for that. ;)

I made some changes and pushed this to project_issue: http://drupalcode.org/project/project_issue.git/commit/bfe445c
See attached patch and interdiff. In summary:

- This code wasn't using the same logic to figure out the human-readable comment identifier as the rest of project_issue, so they were off from each other by default, and it got even worse if you deleted comments in a thread, etc. So, I moved the logic into a helper that both this and our comment_preprocess() could share.

- project_issue_generate_cid_jumplink() seemed like a slightly misleading name for what that function does, especially since not all files have a cid. Renamed it to project_issue_generate_file_jumplink() which seemed more self-documenting.

- Fixed the @return in the PHP doc for that function.

- The PHP doc @param ($file) didn't match the actual function parameter ($item). $file seemed more self-documenting, so I went with that.

Otherwise, all seems to work fine with the D7 field_formatter_settings contrib.

I split off the d.o-specific stuff to #1954454: Use extended_file_field for issue node file fields for d.o D7 port so let's continue there.

Thanks!
-Derek

FYI #1954454: Use extended_file_field for issue node file fields for d.o D7 port is now done, so a new rebuild should be using this without any manual reconfiguration or needing to install anything.

-Derek

Thanks for the review, klonos.

Re: length -- yes, we know. See the issue summary. ;) In particular: #1833684: Need a file widget that gracefully handles lots of hidden files

Re: order of fields: a bit premature to be worrying about that. We're not yet at the polish all the visuals stage. Ditto field labels.

Re: interdiff and grouping related files: please open a whole separate feature request about that.

Re: Testbot -- yes, we know. See #1760658: Come up with a formatter for file fields and other references to the test bot in this issue.

Re: Block placement/behavior: Yes, probably better in #1545922: [META] Issue page redesign.

Cheers,
-Derek