TODO: NEEDS CHANGE NOTIFICATION

ISSUE #1:
theme_image_formatter() unneccessarily copies field-by-field from $item to $image, and loses $item['attributes'] in the process.

ISSUE #2:
Initial patches introduced $item['path'] = $item['uri'], which flagged that the path->uri renaming in the File API was not brought over to the Image system.

SOLUTION:
Final patch removes the $item to $image field mapping, passing along $item instead.
Patch also updates the Image system to use the 'uri' key instead of the 'path' key, following the pattern set in File API.
Since we don't want an API change (ie "path" to "uri") in D7, we just add the ability pass "attributes" parameter through to the $item. This is being pursued in a separate issue (#1329586).

NEXT STEPS:
1. Suggestion expressed that this key should actually be 'src'. Recommend new issue be opened for that discussion, rather than delaying the 'attributes' fix further.

Original Issue Text
When trying to add custom css class to image field in my node--[some_type].tpl.php, I discovered it does not work as it seems it should.

#in node--[some_type].tpl.php

foreach ($content['field_image'] as $k => $v) {
  $v['#item']['attributes']['class'] = array('zzz');
  print render($v);
  # expected output: <img class="zzz" src="..." alt="..." ... />
  # actual output: <img src="..." alt="..." ... />
}

By print_r'ing I discovered that in image.field.inc following code dismissed attributes field. Btw why copy from $item to $image field-by-field rather than pass $item to theme()?

function theme_image_formatter($variables) {
  $item = $variables['item'];
  $image = array(
    'path' => $item['uri'],
    'alt' => $item['alt'],
  );
  // Do not output an empty 'title' attribute.
  if (drupal_strlen($item['title']) > 0) {
    $image['title'] = $item['title'];
  }

  if ($variables['image_style']) {
    $image['style_name'] = $variables['image_style'];
    $output = theme('image_style', $image);
  }
  else {
    $output = theme('image', $image);
  }

  if ($variables['path']) {
    $path = $variables['path']['path'];
    $options = $variables['path']['options'];
    // When displaying an image inside a link, the html option must be TRUE.
    $options['html'] = TRUE;
    $output = l($output, $path, $options);
  }

  return $output;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

Version: 7.0 » 7.x-dev

can you reroll a patch for testbots
http://drupal.org/patch/create

thanks.

RobLoach’s picture

This is definitely an issue as it means that image_formatters can't grab classes and any other things that would be nice to inject.

536,540d535
<
<   if ($item['attributes']) {
<       $image['attributes'] = $item['attributes'];
<   }
<

Any thoughts on sending in $item as the $image instead of creating a new one? Then anything that you would like to send into the theme function would become available.

RobLoach’s picture

Status: Active » Needs review
FileSize
1.31 KB

How about this? Also applies to 8.x too.... This would be very helpful to both YoxView and Hover Preview.

Status: Needs review » Needs work

The last submitted patch, theme_image_formatter.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
560 bytes

Yeah, that might be better for the 8.x branch. 7.x should just inject the attributes property directly.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Tested once again, works. With this patch, you can pass attributes to the #item in the image formatter.

rfay’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

D8 first. Let's get these fixed on D8 and into D7.

RobLoach’s picture

Status: Needs review » Needs work

For Drupal 8, I think we should send the whole $item object into theme('image'), so that it gets all the "image information". For Drupal 7, we could talk about having it just do #attributes, or the whole $item... Worth considering.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

With this patch, image formatters can take the attributes and cleans it up a bit in the process.

RobLoach’s picture

FileSize
560 bytes

The Drupal 7 patch just injects the attributes manually.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

We're using this on http://acquia.com/customers to inject data-hover-preview attributes to the image tags.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/modules/image/image.field.inc
@@ -528,21 +528,14 @@ function image_field_formatter_view($entity_type, $entity, $field, $instance, $l
+  $item['path'] = $item['uri'];

That statement suggests there might be more small clean-ups. Looks like we need to unnecessarily copy a variable.

RobLoach’s picture

FileSize
18.66 KB

It looks like the "path" to "uri" change was from the File API path/uri renaming, but it wasn't brought over to the Image system. This updated patch updates all theme('image') calls from path to uri instead.

With this path to uri rename, we don't need to copy the variable. It actually ends up being less code.

RobLoach’s picture

FileSize
20.77 KB

Missed a rename in image_style.

RobLoach’s picture

Status: Needs work » Needs review

Testing bot?

Status: Needs review » Needs work

The last submitted patch, 1025796.patch, failed testing.

c4rl’s picture

Subscribearino

effulgentsia’s picture

Subscribe.

jantoine’s picture

subscribe

*edit
Patch in #5 still applies cleanly and works.

thehong’s picture

Subcribe.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
FileSize
20.69 KB

Attempted reroll after the /core move. Likely to fail, so assigning for follow-ups.

Status: Needs review » Needs work

The last submitted patch, drupal-1025796-21.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review
FileSize
20.62 KB

Another re-roll, which hopefully doesn't introduce too many other issues while addressing the undefined index issues in the previous test.

Status: Needs review » Needs work

The last submitted patch, drupal-1025796-23.patch, failed testing.

tim.plunkett’s picture

I had originally opened #1329586: theme_image_formatter should pass along attributes before realizing it was a duplicate, but it's basically the same idea as #9/#10. Do you think it's reasonable to get those easy fixes in now, and then split off the giant rename-path-to-uri for D8 only?

jthorson’s picture

Status: Needs work » Needs review
FileSize
20.96 KB

Perhaps ... though in one case, it appears that Rob's patch from #14 actually wipes out the $image variable and replaces it with $item, at which point your $image variable in #1329586-1: theme_image_formatter should pass along attributes doesn't actually get referenced - so would require the rework in either case.

In any case, this bug bit me on a recent project, so I was just chipping in some effort to try and get your patch back to the results in #14 (though I don't understand where the one remaining error in #14 was coming from).

Here's another attempt, which I think takes care of the remaining exception errors. (Sorry for the issue spam ... don't actually have an environment where I can test locally at the present moment.)

Status: Needs review » Needs work

The last submitted patch, drupal-1025796-26.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review
FileSize
20.96 KB

Swapping an array key to address the field display test failures ... and so that I can find my current status if/when I come back to tackle this again.

Status: Needs review » Needs work

The last submitted patch, drupal-1025796-28.patch, failed testing.

tim.plunkett’s picture

Title: theme_image_formatter ignores attributes » Rename path to uri in image functions
Assigned: tim.plunkett » Unassigned
Issue tags: -attributes

Let's get #1329586: theme_image_formatter should pass along attributes into both D7 and D8, then rewrite all of these variables.

jthorson’s picture

Status: Needs work » Needs review
FileSize
22.37 KB

Rob's patch added a '!empty()' check to each of the standard image attributes ... however, based on http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_image/7 the "alt" attribute currently defaults to an empty string unless we explicitly set it to NULL ... so I've removed that check.

With that removed, it was necessary to set an empty 'title' attribute to a few of the $image_info definitions in order for the tests to pass. I believe this is due to the 'title' => $this->randomName(), code in uploadNodeImage() (inside of image.test) ... but since I'm able to match this by adding 'title'='' to the definitions during the actual tests, I suspect that this line is setting the title attribute to an empty string.

From Rob's patch, the final failing test was due to one missed path->uri swap.

In any case, I think the attached should now pass all tests ...

jthorson’s picture

Just noticed the following code which was stripped out of theme_image_formatter() (file image.field.inc), and which we may want to put back in to get rid of the 'title' => ''lines I had to put in to get tests passing successfully ...

  // Do not output an empty 'title' attribute.
  if (drupal_strlen($item['title']) > 0) {
    $image['title'] = $item['title'];
  }

Unfortunately, by doing this, I ended up with failures in the ImageFieldDisplayTestCase suite again.

jthorson’s picture

FileSize
22.37 KB

Errors experienced in #32 were due to me passing the point of the night where the brain stops working. ;)

This one should be good, without the 'title' => '' hacks.

Dave Reid’s picture

Has anyone discussed making this variable named 'src' instead so that it matches the actual attribute of the image tag? It makes a lot more sense for me to do theme('image', array('src' => 'public://myimage.jpg')); than theme('image', array('uri' => 'public://myimage.jpg'));. We don't use $variables['alternate_text'] we use $variables['alt'] because it matches <img alt="..." />.

File API is not *the only* system using theme('image') and I would hate to see this remain a WTF over not wanting to reassign one little variable. We use it randomly throughout core with non-URI paths (e.g. 'core/misc/arrow-asc.png') as well as it is commonly used by themers. I think both 'uri' or 'path' are WTFs and we should be using a key that is actually easy to understand, which is 'src'. We continue to document that we support multiple types of input: stream wrapper URIs, relative paths, and absolute paths.

jthorson’s picture

Ooops ... looks like the patch in #33 is identical to the patch in #31 ... my latest changes didn't make it in:

TODO:
1. Insert at the top of theme_image_formatter:

if (strlen($item['title'] == 0)) {
  unset $item['title'];
}

2. Go through image.test to remove instances of 'title' => '' from any theme('image') calls.

3. Determine whether the 'title' => $this->randomName(), code in uploadNodeImage() (inside of image.test) is redundant. Node title, not image!

jthorson’s picture

Re: #34 - I could get behind 'src' instead of uri.

That said, my motivation behind cleaning up Rob/Tim's efforts on this patch was to get the D7 backport (in #5) committed so that I can quite hacking core on my current project. Dries's comment at #12 in this issue shifted the focus away from the original problem, which was that theme_image_formatter wasn't passing attributes. It might be nice to get this in sooner for the sake of the attributes backport, and then chase the key naming in a new issue.

Alternatively, the same D7 attributes patch can be found in #1329586: theme_image_formatter should pass along attributes, where it needs tests before it would be ready to commit to both D7 and D8 ... after which a rework of this patch would remove most of the code again in the D8 codebase, but the D7 version would finally be in and addressing the bug.

jthorson’s picture

FileSize
22.08 KB

This patch includes the stuff which was supposed to be in #33.

Dries’s picture

This patch looks good to me, assuming it comes back green.

Status: Needs review » Needs work

The last submitted patch, drupal-1025796-37.patch, failed testing.

jthorson’s picture

FileSize
22.13 KB

Typo in the strlen($item['title']) check. Verified locally that this version passes the 12 which failed in the last patch ... should be green again, unless there are other tests which are validating against img tags with an empty title attribute.

jthorson’s picture

Status: Needs work » Needs review

Status for testbot.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good and works, and it came back green to boot! :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks good to me as well. Committed/pushed to 8.x, will need porting to 7.x.

RobLoach’s picture

FileSize
560 bytes

Not sure this patch should be back-ported to Drupal 7 as it is an API change, but the sister bug might: #1329586: theme_image_formatter should pass along attributes

tim.plunkett’s picture

Status: Patch (to be ported) » Fixed

Marking as fixed, let's continue in the other issue.

catch’s picture

Version: 7.x-dev » 8.x-dev
catch’s picture

Title: Rename path to uri in image functions » Change notification for: Rename path to uri in image functions
Category: bug » task
Priority: Normal » Critical
Status: Fixed » Active

rfay pinged me in irc to point out this needed an issue summary- which is true since it took a different path from the original patch.

Also, this is an API change, so we should document that (and improve the issue summary for people who get here from git blame).

catch’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Status: Active » Needs work
Issue tags: +Needs issue summary update, +Needs documentation updates, +Needs change record

I have to go, so I can't write up a proper one, buuuuuuuut:

Drupal 8
Change from "path" to "uri" and work with straight up $item in instead of switching to $image.
Drupal 7
Since we don't want an API change (ie "path" to "uri"), we just add the ability pass "attributes" parameter through to the $item. This is over at #1329586: theme_image_formatter should pass along attributes.

Is tagging it "Needs change notification" correct?

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

jthorson’s picture

Status: Needs work » Needs review

Change notice created at http://drupal.org/node/1353146.

jthorson’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

Title: Change notification for: Rename path to uri in image functions » Rename path to uri in image functions
Category: task » bug
Priority: Critical » Normal

For posterity.

jenlampton’s picture

For the record, I think it is silly to expect front end developers to know what the heck a URI is. :/

Pretty please can we follow up on getting this changed to SRC asap :)
see #1643890: Change URI to SRC in theme_image and similar

xjm’s picture

xjm’s picture

Issue summary: View changes

Tweaked issue summary