Here's the theme_image function from theme.inc

 function theme_image($path, $alt = '', $title = '', $attr = '', $getsize = true) {
  if (!$getsize || (file_exists($path) && (list($width, $height, $type, $attr) = @getimagesize($path)))) {
    return "<img src=\"$path\" $attr alt=\"$alt\" title=\"$title\" />";
  }
}

Any arguments sent to the function for $attr get clobbered by the getimagesize() call. And $getsize defaults to true, so in a call to:

theme_image('files/images/foo', 'alt_text', 'title_text', 'align="left"')

the 'align="left"' never makes it to the page. The two attribute values really should be able to coexist, allowing for both getsize attributes and additional 'argumented' attributes as well.

I recommend changing the $attr in the second line to $imgattr, then adding $imgattr to the tag returned in the third.

-Jeff

CommentFileSizeAuthor
#7 theme-img_attr_fix.patch1 KBjjeff
#5 theme_image-attr_fix_2.patch1.34 KBjjeff
#3 theme_image-attr_fix_1.patch1.51 KBjjeff
#2 theme_image-attr_fix_0.patch1.51 KBjjeff
#1 theme_image-attr_fix.patch1.51 KBjjeff

Comments

jjeff’s picture

jjeff commented
Priority: Normal » Critical
StatusFileSize
new theme_image-attr_fix.patch1.51 KB

Okay, here's a patch to fix the theme_image attribute problem.

I've also added a handler so that attributes can either be a string (as they were in the past - although it wasn't working) or an associative array.

I've marked this issue as critical because the current version of theme_image() does not accept attributes at all.

-Jeff

jjeff’s picture

jjeff commented
StatusFileSize
new theme_image-attr_fix_0.patch1.51 KB

Whoops... Found a mistake in my code...

Here's the correction.

-Jeff

jjeff’s picture

jjeff commented
StatusFileSize
new theme_image-attr_fix_1.patch1.51 KB

It's amazing how many mistakes one person can make in two lines of code.

New patch... this one works!

-jeff

Steven’s picture

Steven commented

Attributes should be consistently passed as an array, and rendered with drupal_attributes(). This is to ensure data is escaped correctly.

jjeff’s picture

jjeff commented
StatusFileSize
new theme_image-attr_fix_2.patch1.34 KB

Now the $attr argument requires an array.
And the $attr argument gets sent through drupal_attributes().
Simple and easy, really.

-Jeff

dries’s picture

dries commented

Tidied up the code a little, and committed it to HEAD and DRUPAL-4-6. Thanks.

jjeff’s picture

jjeff commented
StatusFileSize
new theme-img_attr_fix.patch1 KB

Oops... the new fix sets the default value for $attributes to NULL. It should at least be array(), now that $attributes is an array.

This patch fixes the default value and adds some other error checking that should help this error that people may be seeing when they load a page with images:

Invalid argument supplied for foreach() in /Users/jeff/Sites/drupal/includes/common.inc on line 1536

-Jeff

dries’s picture

dries commented

AFAIK, we always use $attributes = NULL without special error-checking (see form_ functions in common.inc, for example). Does this fix a bug? If so, how can we reproduce it? I'd love to test it.

hatseflats’s picture

hatseflats commented

I also get this error:

Invalid argument supplied for foreach() in /includes/common.inc on line 1536

When using image.module CVS and drupal CVS. When viewing an image no Only local images are allowed. tag is outputted.

dries’s picture

dries commented

Do you still get that error? If it is specific to the image.module, we might have to move this bug report.

dries’s picture

dries commented
Anonymous’s picture

(not verified) commented
bmargulies’s picture

bmargulies commented
Version: » 4.6.6

Why is the if statement still present in version

$Id: theme.inc,v 1.228.2.8 2005-08-17 20:03:28 dries Exp

when nothing is done with the results of the call to getimagesize?