theme_image_formatter() assumes that title, alt, and options are always set

Good catch:

$image = image_load('misc/druplicon.png');
theme('image_formatter', array('image' => $image, 'image_style' => NULL, 'path' => array('path' => '/node')));

...will generate a notice.

+    $options = !empty($variables['path']['options'])?$variables['path']['options']:array();

I think it's preferred to use isset rather than empty in this case. Additionally, Drupal coding guidelines require spaces before and after the operators (see http://drupal.org/coding-standards#operators).

Also included #1587870: Undefined index: alt in theme_image_formatter() and Undefined index: title in theme_image_formatter() and #1396164: theme_image_formatter() should check for array key.

Needs a bit more work, and a summary.

I think I need $base_url in there or something.

Hm, can't reproduce locally, must be some prefix issue.

Ah!

<a href="/checkout/yuZakFd9">
vs
<a href="/yuZakFd9">

Hopefully this works. Leaving the debug in for now.

Okay! Here's those patches again, cleaned up, without debugging info.
Once these are in for D8 I will rewrite them for D7.

Added issue summary.

Renamed the class and fixed the assertion messages. Finally ready by my standards.

Just to expedite things.

Mostly this looks great to me.

+++ b/core/modules/image/image.field.incundefined
@@ -589,7 +589,7 @@ function theme_image_formatter($variables) {
-  if (drupal_strlen($item['title']) == 0) {
+  if (isset($item['title']) && drupal_strlen($item['title']) == 0) {

@@ -601,9 +601,9 @@ function theme_image_formatter($variables) {
-  if (!empty($variables['path']['path'])) {
+  if (isset($variables['path']['path'])) {
...
-    $options = $variables['path']['options'];
+    $options = isset($variables['path']['options']) ? $variables['path']['options'] : array();

Should we add any inline comments in here?

+++ b/core/modules/image/image.testundefined
@@ -1493,3 +1493,63 @@ class ImageFieldDefaultImagesTestCase extends ImageFieldTestCase {
+    $this->assertEqual($expected_result, $rendered_element, 'theme_image_formatter() correctly renders link fragment.');

Can we add an article here? "...correctly renders a link fragment."

The title hunk already has a comment, and I added one for the path/options section that I think covers it.
And yeah, an article is a good idea there :)

Minor nitpick. At first, I read the test comment as

"without 'title, alt, or path' options"

instead of

"without 'title', 'alt', or 'path options'"

Not sure if something like this would be clearer:

"without image title, alt text, or link path 'options' parameters."

But as I said, this is a minor nitpick at most ... so bumping to RTBC.

Since #1591434: Convert image tests to PSR-0 is about to move all the image tests around, here's a reroll using a PSR-0 test. Can be committed before or after that other issue.

#21: drupal-1038932-21.patch queued for re-testing.

The image tests are PSR-0 now, so I rerolled to make sure one last time. I touched up the comment according to #20, and fixed the test class name.

+++ b/core/modules/image/image.field.inc
@@ -589,7 +589,7 @@ function theme_image_formatter($variables) {
+  if (isset($item['title']) && drupal_strlen($item['title']) == 0) {

why can't empty() be used here?

+++ b/core/modules/image/image.field.inc
@@ -601,9 +601,11 @@ function theme_image_formatter($variables) {
+    $options = isset($variables['path']['options']) ? $variables['path']['options'] : array();

Seems like a good opprotunity to use the ternary operator, ?:

http://www.php.net/manual/en/language.operators.comparison.php#language....

0 is a valid title, no?

AFAIK, you can't use that ternary with isset(), or else the middle case becomes the result of it. It'd be like saying $options = isset($variables['path']['options']) ? (bool) $variables['path']['options'] : array();

Ah you're right. Thanks for helping me see that. RTBC

Hello! I seem to have run into this problem (Drupal 7.14) and found this issue. What branch should I clone into to apply the patch? I tried git clone --recursive --branch 7.x http://git.drupal.org/project/drupal.git. I feel uncertain about how to patch drupal core.

leanderl,

You can try downloading the 'combined' patch in #24 and applying it to your 7.x branch (using git apply) ... but the patch has not made it into the 7.x codebase yet. (The 'reviewed & tested by the community' status indicates that it's waiting for a core committer to add it ... once you see someone post a 'committed' comment here, then it will be included in a new clone.)

Thx for quick and constructive reply jthorson!

This looks like sensible hardening, and comes with a test. Woo!

Committed and pushed to 8x. This will need a re-roll for D7 thanks to PSR-0.

Rerolled.

Ah yes, this is where D7 and D8 differ. (they set 'alt' completely differently)

Ah, RDF module is on during D7 tests, and clean URLs are off.

This should finally be good to go.

Applied and tested against an up to date 7.x branch and compared to the D8 version. Looks good.

#36: drupal-1038932-36.patch queued for re-testing.

I'm retesting this because I expect it to fail... the fact that clean URLs are off during D7 tests was a recent bug/regression (it was like that for a while in Drupal 8 too), now fixed in both places due to #1541958: Split setUp() into specific sub-methods).

So the test fails for me locally, and I'm guessing it will fail here too, unfortunately.

Also, we might not want to make any assumptions about clean URLs (one way or another) because I think that depends on the environment the test is running in? Perhaps it would be better to change the tests so that they use Xpath to look for the specific attributes they're asserting, rather than looking to match an entire string of HTML?

If it fails, please move it back to D8 so I can switch to xpath.

If it doesn't, then COMMIT IT! :D

Well, it didn't fail, but it should have :) The testbot must be running with clean URLs off or something like that.

Because I tried it again locally, and the patch definitely failed for me when I ran it with clean URLs on, but turning clean URLs off and running the test again makes it pass (see test output below).

If this were a critical bug or something I might say let's commit it and worry about it later :) But since it's not, I think we should stabilize the tests first... sorry. Otherwise most people trying to run these locally are going to wind up getting failures, even if the testbot doesn't.

droth@droth:~/web/drupal$ php scripts/run-tests.sh --url http://localhost/drupal --class ImageThemeFunctionWebTestCase

Drupal test run
---------------

Tests to be run:
 - Image theme functions (ImageThemeFunctionWebTestCase)

Test run started:
 Monday, July 30, 2012 - 00:25

Test summary
------------

Image theme functions 2 passes, 1 fail, and 8 exceptions

Test run duration: 18 sec

droth@droth:~/web/drupal$ drush -y vset clean_url 0
clean_url was set to 0.                                                                                                                                               [success]
droth@droth:~/web/drupal$ php scripts/run-tests.sh --url http://localhost/drupal --class ImageThemeFunctionWebTestCase

Drupal test run
---------------

Tests to be run:
 - Image theme functions (ImageThemeFunctionWebTestCase)

Test run started:
 Monday, July 30, 2012 - 00:26

Test summary
------------

Image theme functions 3 passes, 0 fails, and 8 exceptions

Test run duration: 18 sec

Just confirming that the bot does indeed run with clean URLs off.

Incidently, this is the first time that has caused an unexplained 'pass' instead of an unexplained 'failure'! ;)

Well I know this needs to be fixed in D8, but there is also the need to backport #1696416: Regression: theme_image_style() is broken, and I will have to figure out a way to do this all without making too much work.

One option is to commit this as is, and then open a major follow-up to fix the D8 tests to use xpath, and then backport the theme_image_style tests and the xpath fixes at once.

Switching back to D7 for now just to run the bot.

Actually the "parse exact HTML to prove theme functions work" is systemic, I really do think an xpath follow up is best.

#1175764: Have theme('image_style') inject the style name as a class is a good example of what I mean.

I opened #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output, I think #36 should be committed as is.

This is how other parts of core work around this issue.

This should probably switch to #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output

I don't have any objection to the patch in #47. Using it on a client project and it does what it's supposed to (we were getting some notices emitted because file_entity wasn't passing a title or alt attribute to the theme function and with the patch, that is no longer the case).

The code looks pretty straightforward, so I say RTBC.

I was about to commit this but then noticed there's a subtle behavior change here.

Before the patch, you could explicitly do:

theme('image_formatter', array('item' => array( .... 'alt' => NULL ... )))

in order to force the "alt" attribute not to be printed in the HTML at all. After the patch, though, the "alt" attribute will still be printed, but as an empty string.

That doesn't sound like too big of a deal to me, except that the documentation for theme_image() (which theme_image_formatter() ultimately calls) goes to great lengths to explain that this option is available. So perhaps we don't want to break it?

In any case, preserving the old behavior might be as simple as changing this code to use array_key_exists() instead:

+  if (isset($item['alt'])) {
+    $image['alt'] = $item['alt'];
+  }

Drupal 8 isn't affected by this, just Drupal 7.

Good point. Updated the patch.

I clearly forgot how array_key_exists works.

#53: drupal-1038932-53.patch queued for re-testing.

This addressed #50, and still passes. RTBC anyone?

Looks good, slipped my mind

I'm confused on whether this needs to be held up on #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output or not.

Tim and I talked, and he pointed out that there are many places that could use the new assertThemeOutput(), so doing that more core-wide could be a follow-up, rather than a blocker here. I'm inclined to agree.

Committed and pushed to 7.x. Thanks!