Hi,
I have just upgraded from alpha5 to beta3. Great work on the enhancements and fixes, thanks.

When editing a node, on preview I noticed that the attached image is not being shown in the teaser, but it is shown when the saved node is displayed in teaser from, eg on front page. I tracked through the code and have found where the problem lies - it seems that in some places the array of image ids, $node->iids, is keyed from 0, eg 0=>77, 1=>76 for two images 77 and 76, but in other places it is keyed with the image id, eg 77=>77, 76=>76. The attached screen grabs illustrate this.

  • #1 is from a blog node when displayed on the front page. In image_attach_nodeapi() the array is keyed 0,1 and the first image $node->iids[0] is correctly passed to theme_image_attach_teaser()
  • #2 is the same node in full page, with both image ids passed in turn to theme_image_attach_teaser()
  • #3 shows the values after entering edit mode.
  • #4 is a preview during editing. Now the image array $node->iids is keyed on the image id, which is fine for the full page, because each node is processed with a foreach loop. But when the teaser is rendered with image_attach_nodeapi $node->iids[0] does not exist, so no second parameter is passed to theme_image_attach_teaser() and the image is not rendered.

If you want to replicate this error, enable image_attach for the blog node type, create a new post, attach an image, and preview it. If anyone can reproduce this it would be useful, as I don't think is is anything odd with my site, but it just could be.

I have not yet worked out why the image array is 0-keyed in nodeapi_image_attach when rendered for viewing (picture 1) but keyed on image id when rendered for edit preview (picture). op=view and teaser=1 in both cases. I think it must be something to do with $node->build_mode which is 0 for normal viewing of the teaser on the front page, but 1 when generating the edit preview, as shown in the node_build_content() debug.

However, a simple fix in image_attach_nodeapi() is to use current($node->iids) instead of $node->iids[0] as this will always return an item in this situation.

The attached patch makes the change. Hope this is helpful.

Jonathan

CommentFileSizeAuthor
#2 _image_attach.teaser_firstiid.patch.txt714 bytesjonathan1055
_image_attach.teaser_image.patch.txt695 bytesjonathan1055
image_attach blog 4 preview.jpg315.05 KBjonathan1055
image_attach blog 3 on edit.jpg18.04 KBjonathan1055
image_attach blog 2 full node.jpg59.09 KBjonathan1055
image_attach blog 1 front page.jpg50.79 KBjonathan1055

Comments

joachim’s picture

joachim commented
Status: Needs review » Needs work

Good catch!
And thanks for doing the detective work :)

I'm able to reproduce this, but you need enough text to cause the trimmed preview to be shorter than the full node.

The reason for the difference in keying is that a select box form item in FormAPI returns an array whose keys are the ids from the things you're selecting, and whose values are 0 for unselected and the same id again if selected.
So:
7 => 0
8 => 8
means the select box has nodes 7 and 8, and only 8 is selected.
But when we are looking at a rendered node, the attached images are keyed by weight, so 0-n.

I'd rather use something other than count(), because although we're unlikely to mess with the array pointer, it doesn't feel like a robust way to get the first value.
A quick read of PHP docs suggests reset(). (Which strikes me as ugly. Why a language with 5 million functions can't spare a synonym likst 'first' I don't know.)

It's a shame we can't AFAIK detect when the node is in preview.

Something like this seems to me to be most readable:

          $first_iid = reset($node->iids);
          $node->content['image_attach'][$first_iid]['#value'] = theme("image_attach_{$teaser_or_body}", $node, $first_iid);

Will reroll this when I get a moment.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review
StatusFileSize
new _image_attach.teaser_firstiid.patch.txt714 bytes

Yes, you are right, creating $first_iid is clearer and safer. Here is a patch that does exactly the code change above, to make it easier for others to test, and for you to implement. It is rolled against dev of 2009-11-07

Jonathan

joachim’s picture

joachim commented
Status: Needs review » Fixed

Thanks for rolling the patch again!

Works perfectly for me.
I've added a comment to about the use of reset to prevent future confusion about its reason for being there, and committed to CVS.

#630546 by jonathan1055: Fixed attached image not appearing in teaser preview.

Status: Fixed » Closed (fixed)

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