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;
}
Files: 
CommentFileSizeAuthor
#44 1025796-d7.patch560 bytesRobLoach
#40 drupal-1025796-40.patch22.13 KBjthorson
PASSED: [[SimpleTest]]: [MySQL] 34,020 pass(es).
[ View ]
#37 drupal-1025796-37.patch22.08 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] 34,002 pass(es), 12 fail(s), and 0 exception(es).
[ View ]
#33 drupal-1025796-33.patch22.37 KBjthorson
PASSED: [[SimpleTest]]: [MySQL] 33,977 pass(es).
[ View ]
#31 drupal-1025796-31.patch22.37 KBjthorson
PASSED: [[SimpleTest]]: [MySQL] 33,988 pass(es).
[ View ]
#28 drupal-1025796-28.patch20.96 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] 33,972 pass(es), 15 fail(s), and 0 exception(es).
[ View ]
#26 drupal-1025796-26.patch20.96 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] 33,967 pass(es), 15 fail(s), and 0 exception(es).
[ View ]
#23 drupal-1025796-23.patch20.62 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] 33,969 pass(es), 15 fail(s), and 9 exception(es).
[ View ]
#21 drupal-1025796-21.patch20.69 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 33,956 pass(es), 12 fail(s), and 21 exception(es).
[ View ]
#14 1025796.patch20.77 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 29,433 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#13 1025796.patch18.66 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 29,425 pass(es), 4 fail(s), and 18 exception(es).
[ View ]
#10 1025796-d7.patch560 bytesRobLoach
#9 1025796.patch1.53 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 29,397 pass(es).
[ View ]
#5 1025796.patch560 bytesRobLoach
PASSED: [[SimpleTest]]: [MySQL] 31,533 pass(es).
[ View ]
#3 theme_image_formatter.patch1.31 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 31,567 pass(es), 10 fail(s), and 0 exception(es).
[ View ]
image.field_.inc_.diff109 bytesalex_ustinov
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in image.field_.inc_.diff.
[ View ]

Comments

Version:7.0» 7.x-dev

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

thanks.

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.

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

Status:Active» Needs review
StatusFileSize
new1.31 KB
FAILED: [[SimpleTest]]: [MySQL] 31,567 pass(es), 10 fail(s), and 0 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new560 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,533 pass(es).
[ View ]

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

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.53 KB
PASSED: [[SimpleTest]]: [MySQL] 29,397 pass(es).
[ View ]

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

StatusFileSize
new560 bytes

The Drupal 7 patch just injects the attributes manually.

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.

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.

StatusFileSize
new18.66 KB
FAILED: [[SimpleTest]]: [MySQL] 29,425 pass(es), 4 fail(s), and 18 exception(es).
[ View ]

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.

StatusFileSize
new20.77 KB
FAILED: [[SimpleTest]]: [MySQL] 29,433 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Missed a rename in image_style.

Status:Needs work» Needs review

Testing bot?

Status:Needs review» Needs work

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

Subscribearino

Subscribe.

subscribe

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

Subcribe.

Assigned:Unassigned» tim.plunkett
Status:Needs work» Needs review
StatusFileSize
new20.69 KB
FAILED: [[SimpleTest]]: [MySQL] 33,956 pass(es), 12 fail(s), and 21 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new20.62 KB
FAILED: [[SimpleTest]]: [MySQL] 33,969 pass(es), 15 fail(s), and 9 exception(es).
[ View ]

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.

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?

Status:Needs work» Needs review
StatusFileSize
new20.96 KB
FAILED: [[SimpleTest]]: [MySQL] 33,967 pass(es), 15 fail(s), and 0 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new20.96 KB
FAILED: [[SimpleTest]]: [MySQL] 33,972 pass(es), 15 fail(s), and 0 exception(es).
[ View ]

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.

Title:theme_image_formatter ignores attributesRename 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.

Status:Needs work» Needs review
StatusFileSize
new22.37 KB
PASSED: [[SimpleTest]]: [MySQL] 33,988 pass(es).
[ View ]

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 ...

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.

StatusFileSize
new22.37 KB
PASSED: [[SimpleTest]]: [MySQL] 33,977 pass(es).
[ View ]

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.

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.

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!

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.

StatusFileSize
new22.08 KB
FAILED: [[SimpleTest]]: [MySQL] 34,002 pass(es), 12 fail(s), and 0 exception(es).
[ View ]

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

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.

StatusFileSize
new22.13 KB
PASSED: [[SimpleTest]]: [MySQL] 34,020 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Status for testbot.

Status:Needs review» Reviewed & tested by the community

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

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.

StatusFileSize
new560 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

Status:Patch (to be ported)» Fixed

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

Version:7.x-dev» 8.x-dev

Title:Rename path to uri in image functionsChange 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).

Issue summary:View changes

Updated issue summary.

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?

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review

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

Status:Needs review» Fixed

Status:Fixed» Closed (fixed)

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

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

For posterity.

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

Issue summary:View changes

Tweaked issue summary