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;
}
Comment | File | Size | Author |
---|---|---|---|
#44 | 1025796-d7.patch | 560 bytes | RobLoach |
#40 | drupal-1025796-40.patch | 22.13 KB | jthorson |
#37 | drupal-1025796-37.patch | 22.08 KB | jthorson |
#33 | drupal-1025796-33.patch | 22.37 KB | jthorson |
#31 | drupal-1025796-31.patch | 22.37 KB | jthorson |
Comments
Comment #1
droplet CreditAttribution: droplet commentedcan you reroll a patch for testbots
http://drupal.org/patch/create
thanks.
Comment #2
RobLoachThis is definitely an issue as it means that image_formatters can't grab classes and any other things that would be nice to inject.
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.
Comment #3
RobLoachHow about this? Also applies to 8.x too.... This would be very helpful to both YoxView and Hover Preview.
Comment #5
RobLoachYeah, that might be better for the 8.x branch. 7.x should just inject the attributes property directly.
Comment #6
RobLoachTested once again, works. With this patch, you can pass attributes to the
#item
in the image formatter.Comment #7
rfayD8 first. Let's get these fixed on D8 and into D7.
Comment #8
RobLoachFor 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.
Comment #9
RobLoachWith this patch, image formatters can take the attributes and cleans it up a bit in the process.
Comment #10
RobLoachThe Drupal 7 patch just injects the attributes manually.
Comment #11
RobLoachWe're using this on http://acquia.com/customers to inject
data-hover-preview
attributes to the image tags.Comment #12
Dries CreditAttribution: Dries commentedThat statement suggests there might be more small clean-ups. Looks like we need to unnecessarily copy a variable.
Comment #13
RobLoachIt 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
touri
rename, we don't need to copy the variable. It actually ends up being less code.Comment #14
RobLoachMissed a rename in image_style.
Comment #15
RobLoachTesting bot?
Comment #17
c4rl CreditAttribution: c4rl commentedSubscribearino
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedSubscribe.
Comment #19
jantoine CreditAttribution: jantoine commentedsubscribe
*edit
Patch in #5 still applies cleanly and works.
Comment #20
thehong CreditAttribution: thehong commentedSubcribe.
Comment #21
tim.plunkettAttempted reroll after the /core move. Likely to fail, so assigning for follow-ups.
Comment #23
jthorson CreditAttribution: jthorson commentedAnother re-roll, which hopefully doesn't introduce too many other issues while addressing the undefined index issues in the previous test.
Comment #25
tim.plunkettI 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?
Comment #26
jthorson CreditAttribution: jthorson commentedPerhaps ... 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.)
Comment #28
jthorson CreditAttribution: jthorson commentedSwapping 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.
Comment #30
tim.plunkettLet's get #1329586: theme_image_formatter should pass along attributes into both D7 and D8, then rewrite all of these variables.
Comment #31
jthorson CreditAttribution: jthorson commentedRob'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 ...
Comment #32
jthorson CreditAttribution: jthorson commentedJust 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 ...Unfortunately, by doing this, I ended up with failures in the ImageFieldDisplayTestCase suite again.
Comment #33
jthorson CreditAttribution: jthorson commentedErrors 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.Comment #34
Dave ReidHas 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'));
thantheme('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.
Comment #35
jthorson CreditAttribution: jthorson commentedOoops ... 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:
2. Go through image.test to remove instances of
'title' => ''
from any theme('image') calls.3. Determine whether theNode title, not image!'title' => $this->randomName(),
code in uploadNodeImage() (inside of image.test) is redundant.Comment #36
jthorson CreditAttribution: jthorson commentedRe: #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.
Comment #37
jthorson CreditAttribution: jthorson commentedThis patch includes the stuff which was supposed to be in #33.
Comment #38
Dries CreditAttribution: Dries commentedThis patch looks good to me, assuming it comes back green.
Comment #40
jthorson CreditAttribution: jthorson commentedTypo 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.Comment #41
jthorson CreditAttribution: jthorson commentedStatus for testbot.
Comment #42
tim.plunkettThe patch looks good and works, and it came back green to boot! :)
Comment #43
catchLooks good to me as well. Committed/pushed to 8.x, will need porting to 7.x.
Comment #44
RobLoachNot 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
Comment #45
tim.plunkettMarking as fixed, let's continue in the other issue.
Comment #46
catchComment #47
catchrfay 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).
Comment #47.0
catchUpdated issue summary.
Comment #48
RobLoachI have to go, so I can't write up a proper one, buuuuuuuut:
$item
in instead of switching to$image
.$item
. This is over at #1329586: theme_image_formatter should pass along attributes.Is tagging it "Needs change notification" correct?
Comment #48.0
RobLoachUpdated issue summary.
Comment #49
jthorson CreditAttribution: jthorson commentedChange notice created at http://drupal.org/node/1353146.
Comment #50
jthorson CreditAttribution: jthorson commentedComment #52
tim.plunkettFor posterity.
Comment #53
jenlamptonFor 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
Comment #54
xjmComment #54.0
xjmTweaked issue summary