I've got a collection of banner images that are all the same aspect ratio but different dimensions, all of which I want to auto-scale into a

by specifying <img width="100%" ...> and leaving out the height attribute. I'm using a liquid theme, and the
changes width based on browser window dimensions. But image_display() grabs data from image_get_info() and sets image dimensions in pixels, so the banner can't resize to fit the window.

So ... from time to time, I'd like to be able to let the browser figure out the layout to use for an image, rather than specifying height and width in pixels.

CommentFileSizeAuthor
#7 image-424556.patch1.27 KBpebosi
#1 image-6.x-1.x-dev-424556-1.patch1.21 KBghoti

Comments

ghoti’s picture

ghoti commented
Status: Active » Needs review
StatusFileSize
new image-6.x-1.x-dev-424556-1.patch1.21 KB

The attached patch fixes this by allowing height and width to be passed as $attributes to image_display(). If an empty attribute is passed, it gets unset() so that it will be eliminated from the eventual HTML output.

ghoti’s picture

ghoti commented

For reference, with this patch, image_display could be called with something like this:


  // Old familiar notation, behaves as it always has.
  print image_display( $image, '_original', array( 'title' => 'Some picture...' ) );

  // Width will be forced to "100%", but height will still be grabbed via image_get_info().
  // This is probably NOT what you want to do, as it'll make the aspect ratio wonky.
  print image_display( $image, '_original', array( 'width' => '100%' ) );

  // Set the width to "100%", but by setting the height to an empty string, cause the
  // function to unset() the attribute, allowing the browser to calculate its dimensions.
  print image_display( $image, '_original', array( 'width' => '100%', 'height' => '' ) );

Of course, other attributes can still be supplied as they could before.

joachim’s picture

joachim commented

My take on this is still that it's best done with CSS in your theme.

I'd like something less noisy in the patch though. Could we do all the caller overriding in one go?
Maybe load up $image_info_attributes with the stuff we get from $image_info, and then do:
$attributes = $attributes + $image_info_attributes

I'm not up on how PHP handles clobbering array keys -- we'd want the passed-in $attributes to win, basically.
Also, we want the class attribute to marge not clobber -- maybe append our own classes after the array merging.

ghoti’s picture

ghoti commented

While I agree that in doing this in CSS is generally preferable, there will still be cases where someone will want to control image display while using a default theme. And hey, the cost of a couple of extra if's per image seems cheap...

Regarding passed-in attributes, are you suggesting that if someone passes array('height'=>''), then we should actually include an empty height attribute in the Only local images are allowed. that gets sent to HTML? If so, then I disagree; you've got the protection against that already in the function, and the reasoning that put it there is sound. Other than that, as the patch stands, passed-in attributes *do* override anything learned from image_get_info().

It seems to me that the only way to avoid clobbering keys is to check for their existence before adding them, which we're doing with this patch. The only attributes that image_display() adds itself are width and height. The image_get_info() function returns other data that are *not* appropriate as attributes in an Only local images are allowed., so just merging the two arrays together wouldn't work.

Regarding the noise, I tried to figure out how to shrink the if-nest but I couldn't. This is the tightest I could make it while capturing the logic required. Any other suggestions would be welcome.

joachim’s picture

joachim commented
Status: Needs review » Needs work

This is what I had in mind:


$args = array(
  'clobber' => 'args',
  'extra' => 'args',
  'kill' => NULL,
);
  
$ours = array(
  'clobber' => 'ours',
  'kill' => 'ours',
  'safe' => 'ours',
);


$result = array_merge($ours, $args);

print_r($result);

Our class would then need to be appended.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
$attributes = array(...passed-in values here...);
if (...) {
  // Determine default width/height, but skip NULL
  $default = array(...);
}
$attributes += $default;
pebosi’s picture

pebosi commented
StatusFileSize
new image-424556.patch1.27 KB

i created a patch, which uses array_merge to combine image_info with attributes and remove empty values with array_filter. please review

regards

pebosi’s picture

pebosi commented
Status: Needs work » Needs review
joachim’s picture

joachim commented
Status: Needs review » Needs work

I've had a closer look at what's going on here.
We can't simply replace these lines with array merging:

-  // Only output width/height attributes if image_get_info() was able to detect
-  // the image dimensions, since certain browsers interpret an empty attribute
-  // value as zero.
-  if (!empty($image_info['width'])) {
-    $attributes['width'] = $image_info['width'];
-  }

These bits can only pass data into the new array if it's not empty, as it says in the comments.