I don't know exactly the policy regarding theming of core modules, but I think attachment list in upload.module should be themable. Here is a small patch to do so.

--- upload.module.orig  2005-05-22 10:23:58.784086137 +0200
+++ upload.module       2005-05-22 10:24:42.282575193 +0200
@@ -239,17 +239,13 @@

     case 'view':
       if ($node->files && user_access('view uploaded files')) {
-        $header = array(t('Attachment'), t('Size'));
-        $rows = array();
         $previews = array();
+        $filelist = array();

         // Build list of attached files
         foreach ($node->files as $file) {
           if ($file->list) {
-            $rows[] = array(
-              '<a href="/'. ($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path()))) . '">'. $file->filename .'</a>',
-              format_size($file->filesize)
-            );
+            $filelist[] = $file;
             // We save the list of files still in preview for later
             if (!$file->fid) {
               $previews[] = $file;
@@ -273,8 +269,8 @@

         $teaser = $arg;
         // Add the attachments list
-        if (count($rows) && !$teaser) {
-          $node->body .= theme('table', $header, $rows, array('id' => 'attachments'));
+        if (count($filelist) && !$teaser) {
+          $node->body .= theme('node_attachments', $filelist);
         }
       }
       break;
@@ -314,6 +310,18 @@
   return $output;
 }

+function theme_node_attachments($files) {
+   $header = array(t('Attachment'), t('Size'));
+   $rows = array();
+   foreach($files as $file) {
+     $rows[] = array(
+       '<a href="/'. ($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path()))) . '">'. $file->filename .'</a>',
+       format_size($file->filesize)
+     );
+   }
+   return theme('table', $header, $rows, array('id' => 'attachments'));
+}
+
 function upload_count_size($uid = 0) {
   if ($uid) {
     $result = db_query("SELECT SUM(f.filesize) FROM {files} f INNER JOIN {node} n ON f.nid = n.nid WHERE uid = %d", $uid);
CommentFileSizeAuthor
#15 upload.module_36.patch1.25 KBZach Harkey
#10 upload.module_35.patch1.17 KBZach Harkey
#7 upload.module.diff.txt3.74 KBdrumm
#6 patch_343.73 KBmoshe weitzman
#4 23373-4.7.patch.txt3.44 KBDave Cohen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Zach Harkey’s picture

Zach Harkey CreditAttribution: Zach Harkey commented

+1

Tobias Maier’s picture

Tobias Maier CreditAttribution: Tobias Maier commented

is this for 4.6 of head?
we only accept new feature in head and after 4.7

Veggieryan’s picture

Veggieryan CreditAttribution: Veggieryan commented

can we get this updated for 4.7b5?

thanks
ryan grace

Dave Cohen’s picture

Dave Cohen CreditAttribution: Dave Cohen commented
FileSize
23373-4.7.patch.txt3.44 KB

Here's my version for 4.7.

dopry’s picture

dopry CreditAttribution: dopry commented
Version: 4.6.0 » 4.7.0-rc1
Status: Active » Reviewed & tested by the community

I tested on 4.7rc1. The patch at the least doesn't break anything and enables an important feature for themers and site supporting media...

Now people can theme attachments according to extensions... I hope you can figure out what that means to the cunning themer....

Update version to 4.7rc1....

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Category: feature » bug
FileSize
patch_343.73 KB

i tested this patch and it works which is unsuprising since it just moves some HTML into a themeable function. I consider outputting raw HTML without themeability to be a bug.

rerolled for fuzz free

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
FileSize
upload.module.diff.txt3.74 KB

Here is a cleaned up version of this patch. I fixed indentation and made a couple comments more consistent (just because we are programmers doesn't mean we don't do proper english capitalization).

There is more code inside the check for clean URLs then before. I would like to make sure that this has been tested with clean URLs both on and off.

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented

tested and applied.

I've found a minor issue while testing, but it was unrelated.

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Reviewed & tested by the community » Fixed

n

Zach Harkey’s picture

Zach Harkey CreditAttribution: Zach Harkey commented
FileSize
upload.module_35.patch1.17 KB

While allows theming of the actual attachments table, the table is still forcibly and inflexibly appended to the node body.

This patch adds two additional theme functions: theme_upload_teaser() and theme_upload_body(). This convention, originally implemented in image.module, allows much greater flexibility with regard to how the attachment table relates to the node itself.

Possible uses (all actual cases):

  • Display the attachments table above the node body rather than below it. I could also put it in a second column or sidebar.
  • Easily include, (or implement logic to include) the attachments table in the display of the teaser.
  • Change the location and display of the attachments table on a per node-type basis by using theme_upload_attachments directly within any of the node templates. For example in a book node, I could display the attachments _above_ the prev/next navigation instead of being forced beneath the navigation: 
    <?php if (count($node->files) && !$teaser):  ?>
  <div class="downloads">
  <?php print theme('upload_attachments', $node->files); ?>
  </div>
<?php endif; ?>

I design themes for a lot of corporate and commercial websites and one of the things that I find most frustrating is when modules concatenate their output to the node body without leaving any way for me to separate them without resorting to hacks.

bradlis7’s picture

bradlis7 CreditAttribution: bradlis7 commented
Status: Fixed » Active

Opening for discussion.

Dave Cohen’s picture

Dave Cohen CreditAttribution: Dave Cohen commented

While I have no problem with the concept, I think that

a) this could have been submitted as a new bug, and

2) your call to
$node->teaser = theme('upload_teaser', $node);
Is within an if clause that reads
if (count($node->files) && !$teaser)
I don't think this will properly handle the case when $teaser is true. (although it may appear to work for you, since you do nothing to the teaser).

Zach Harkey’s picture

Zach Harkey CreditAttribution: Zach Harkey commented

That code is not part of the submitted patch, it's just an example of the kind of logic that would now be possible at the theme template level, In this particular case node-book.tpl.php — because for book nodes, I wanted to restrict the attachments table to the full view. I could just as easily add an else clause or move that check to somewhere else in the template entirely.

Zach Harkey’s picture

Zach Harkey CreditAttribution: Zach Harkey commented

Wait — yogadex, I misread your comment. Now I see what you're saying and that is a good point. One that I will contemplate over a tasty beer.

Zach Harkey’s picture

Zach Harkey CreditAttribution: Zach Harkey commented
FileSize
upload.module_36.patch1.25 KB

This patch changes
if (count($node->files) && !$teaser)
to
if (count($node->files))
so it now works consistently in both teaser and body views.

I'm including this as part of this issue because I felt that while it wasn't really a new issue but more of an extension of the original issue. Then again, I'm still learning about how to contribute patches so if you guys want me to create a new issue I'd be glad to.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Active » Closed (works as designed)

This is the intended behavior, files aren't listed in teasers for standard themes, but a link is provided.

Zach Harkey’s picture

Zach Harkey CreditAttribution: Zach Harkey commented

You can argue this being the intended behavior for teasers (although I don't see what's wrong with having this flexibility, I know I've needed it in the past), however the ability for theme designers to to separate the attachments table from the node->body is otherwise impossible.