theme_image_formatter() should check for array key [#1396164]

theme_image_formatter() function should check does an array key $variables['path']['options'] exists to avoid Drupal error message.

If you have path for link you must specify $variables['path']['options'], even as empty array to avoid Drupal error message Notice: Undefined index: options in theme_image_formatter() (line 603 of /var/www/public_html/modules/image/image.field.inc).

It should be included check for an array key in function...

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

Comment	File	Size	Author
#1	array_key_check-1396164-1.patch	625 bytes	draganeror

Comments

Comment #1

draganeror

Serbian

Novi Sad

commented 6 January 2012 at 10:25

Status	File	Size
new	array_key_check-1396164-1.patch	625 bytes

I create patch for this.

Comment #2

draganeror

Serbian

Novi Sad

commented 7 January 2012 at 18:32

Status:

Active

» Needs review

Comment #3

draganeror

Serbian

Novi Sad

commented 12 January 2012 at 20:25

Status:

Needs review

» Reviewed & tested by the community

Comment #4

xjm

she/her

English

commented 17 January 2012 at 21:31

Version:	7.10	» 8.x-dev
Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs backport to D7

Thanks for the patch @el22or!

Generally, you shouldn't mark your own patch "Reviewed and Tested by Community"; the patch should go through the review process first. See the issue statuses documentation for more information. Also, per the backport policy, we should create Drupal 8 patches first.

Looking at the patch, I think maybe we need to look at why the options key is not being passed. According to the API documentation, it sounds like the options key is expected:

$variables An associative array containing:

item: An array of image data.

image_style: An optional image style.

path: An array containing the link 'path' and link 'options'.

(Emphasis added.)

So, could you provide steps to reproduce this error? That will help determine what is passing bad data to the function.

Comment #5

draganeror

Serbian

Novi Sad

commented 17 January 2012 at 22:04

This is my first try to make a patch, sorry for mistakes, I got it now.

So, I found problem when I tried simple image transform to link in THEMENAME_preprocess_node().

  // Ad in block
  if ($variables['type'] == 'ad_in_block') {
    if (!empty($variables['field_link']['und'][0]['url'])) {
      $variables['content']['field_image'][0]['#path']['path'] = $variables['field_link']['und'][0]['url'];
      // I don't need any option but there is an error message withouth this.
      $variables['content']['field_image'][0]['#path']['options'] = array();
    }
  }

Comment #6

xjm

she/her

English

commented 17 January 2012 at 23:22

Hi @el22or,

Thanks for the example code. Based on the current API documentation, I think the correct solution currently is to set the value to array() in the preprocess function (as you have done). So it sounds like we definitely clarify the API documentation for this. (The function seems to indicate that the path is optional, by checking if it's set, but not whether options is.)

I think the more usual pattern in core would be to merge in defaults using $array += array('key' => foo, 'key2' => bar).

Oh, also, an aside: use the LANGUAGE_NONE constant (instead of 'und').

Comment #7

xjm

she/her

English

commented 17 January 2012 at 23:39

I did a little more research. It looks like the fact that the options key is expected here is consistent with theme_link(), which also expects it. See also the comments on that page.

Edit 2: Talked this out a bit with some folks in IRC (@msonnabaum, @merlinofchaos, @NiklasFiekas). Based on the comment linked above, we can probably say theme_link() is irrelevant (since that's not a "real" theme function, but this is). Since we are calling l() and options is optional there, we should allow it be too, as in your patch.

Let's clarify the API documentation to indicate that these keys are optional.