upload.module attachment list should be themable
Damien Tournoud - May 22, 2005 - 08:35
| Project: | Drupal |
| Version: | 4.7.0-rc1 |
| Component: | upload.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | by design |
Description
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);
#1
+1
#2
is this for 4.6 of head?
we only accept new feature in head and after 4.7
#3
can we get this updated for 4.7b5?
thanks
ryan grace
#4
Here's my version for 4.7.
#5
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....
#6
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
#7
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.
#8
tested and applied.
I've found a minor issue while testing, but it was unrelated.
#9
n
#10
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):
<?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.
#11
Opening for discussion.
#12
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).
#13
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.
#14
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.
#15
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.
#16
This is the intended behavior, files aren't listed in teasers for standard themes, but a link is provided.
#17
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.