Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

Title: Need more clarity on how to specify image url to theme_image() » Improve documentation for how to specify image url to theme_image()
Project: Drupal.org infrastructure » Drupal core
Version: » 7.x-dev
Component: Other » theme system
Category: feature » task
brendoncrawford’s picture

Can we get some love on this?

webchick’s picture

Version: 7.x-dev » 6.x-dev
Component: theme system » documentation

Moving to documentation component, and moving down to D6. I think #238681: theme_image() not evaluating properly? is a better approach for D7.

jhodgdon’s picture

Version: 6.x-dev » 7.x-dev
Category: task » bug
Priority: Minor » Normal

Looking at the code, it looks like in Drupal 5/6, theme_image() only works if either getsize is FALSE or if the URL is relative to the base path and it can figure out the image dimensions. The original report is wrong, IMO -- it needs to be $getsize=FALSE to work with absolute URLs.

It is also still working the same in Drupal 7. The issue mentioned above has been closed as a "won't fix".

So this needs to be documented in Drupal 7, 6, and 5.

jhodgdon’s picture

Issue tags: +Novice

Could be a good novice project.

mgriego’s picture

I assume this is what you're looking for... My reading of the code was the same that $getsize needs to be false for fully qualified URLs to work, so I added the appropriate parameter comments for D5/6/7.

Status: Needs review » Needs work

The last submitted patch, theme_image-getsize_doc-D5-225950-5.patch, failed testing.

jhodgdon’s picture

Can you rework this a bit? (I"m talking about the D7 patch, didn't look at the others yet -- let's get that one right first):
- We don't usually use all-caps -- it's a bit shouty. Just say "This must be set..." rather than "This MUST be set".
- There is no $path in the D7 args - it's a component of the variables array
- We should probably mention on the 'path' component as well that you need to set the 'getsize' component to FALSE if you use a full URL

Also, please read the instructions below the file attach about file names for D6/D5, which will avoid having the test bot complain.

amateescu’s picture

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

This doesn't make sense anymore for D7 because the $getsize component was removed in #908282: Remove unnecessary I/O from theme_image().

jhodgdon’s picture

Thanks for checking into that -- you are correct. So we just need a Drupal 6 patch.

daniels220’s picture

Assigned: Unassigned » daniels220
FileSize
808 bytes

Here's my stab at a D6 patch.

jhodgdon’s picture

+ *   If this is a full URL, $getsize must be set to FALSE

Missing a . at the end of the sentence.

+ *   $getsize must be set to false if $path is a full URL.

false -> FALSE

Also, when you attach a patch, please set the status to "needs review", so that we know there's something to review, thanks!

daniels220’s picture

Status: Needs work » Needs review
FileSize
795 bytes

Thanks for the feedback. New patch attached.

jhodgdon’s picture

Status: Needs review » Needs work

patch header needs fixing - wrong file names, can't apply

jhodgdon’s picture

Actually, the text here is not correct. You don't need to set $get_size to FALSE if a URL is used, you need to supply the sizes yourself because $get_size will be ignored... correct?

daniels220’s picture

Status: Needs work » Needs review
FileSize
630 bytes

Not sure on the wording for $getsize, but I noted that it is ignored if $path is a full URL.

jhodgdon’s picture

Ugh, my mistake.

I looked closer at the code and I think you do need to set $get_size to FALSE, because if it's set to TRUE and is_file() returns FALSE, nothing will get printed. Yuck! who wrote this code?!?

daniels220’s picture

You are indeed right. New patch attached with slightly more noticeable warnings about needing to set $getsize = false.

jhodgdon’s picture

Status: Needs review » Needs work

Hmmm.

   *   Either the path of the image file (relative to base_path()) or a full URL.
+ *   If this is a full url, $getsize must be set to FALSE or an empty <img> tag
+ *   will be returned.

- In 2nd sentence, url -> URL.
- I don't think that 2nd sentence is accurate. It looks to me like if $getsize is TRUE, nothing at all is returned (not an empty IMG tag).

  * @param $getsize
  *   If set to TRUE, the image's dimension are fetched and added as width/height attributes.
+ *   Must be set to FALSE if $path is a full URL.
  * @return

Maybe this should mention that TRUE is the default? Also, there should be a blank line between the @param and @return.

daniels220’s picture

Status: Needs work » Needs review
FileSize
902 bytes

Fixed all that.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Now we're talking. Looks good. Thanks for the repeat patching.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, committed, thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Novice

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