[drupal-devel] [bug] theme_image doesn't accept html attributes

[Dries]

Issue status update for http://drupal.org/node/21397

 Project:      Drupal
 Version:      cvs
 Component:    theme system
 Category:     bug reports
 Priority:     critical
 Assigned to:  Anonymous
 Reported by:  jjeff
 Updated by:   Dries
 Status:       patch

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.




Dries



Previous comments:
------------------------------------------------------------------------

April 27, 2005 - 04:45 : jjeff

Here's the theme_image function from theme.inc



<?php
 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:



<?php
 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




------------------------------------------------------------------------

May 2, 2005 - 17:44 : jjeff

Attachment: http://drupal.org/files/issues/theme_image-attr_fix.patch (1.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




------------------------------------------------------------------------

May 2, 2005 - 17:47 : jjeff

Attachment: http://drupal.org/files/issues/theme_image-attr_fix_0.patch (1.51 KB)

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


Here's the correction.


-Jeff




------------------------------------------------------------------------

May 2, 2005 - 18:04 : jjeff

Attachment: http://drupal.org/files/issues/theme_image-attr_fix_1.patch (1.51 KB)

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


New patch... this one works!


-jeff




------------------------------------------------------------------------

May 2, 2005 - 18:16 : Steven

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




------------------------------------------------------------------------

May 5, 2005 - 17:09 : jjeff

Attachment: http://drupal.org/files/issues/theme_image-attr_fix_2.patch (1.34 KB)

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


-Jeff




------------------------------------------------------------------------

May 6, 2005 - 11:02 : Dries

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




------------------------------------------------------------------------

May 8, 2005 - 18:26 : jjeff

Attachment: http://drupal.org/files/issues/theme-img_attr_fix.patch (1 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