theme_image() should check for absolute urls
jscheel - March 25, 2008 - 20:56
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | feature request |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | patch (code needs review) |
Description
When calling theme_image(), the only way to properly display images that are absolute urls, is to set getsize = false. This should happen automatically for external urls. Patch attached will check for an absolute url and go ahead with image theming if it is absolute.
| Attachment | Size |
|---|---|
| theme_image_absolute.patch | 1.07 KB |
#1
#2
strpos() would be a bit faster and more readable, no? otherwise, nice work.
#3
Good point moshe. This patch uses stripos(), and only checks for http at position 0. I axed the ftp protocol because I'm pretty sure you can't even use ftp as an image source. stripos() brings about a 26% speed increase over preg_match(). If we would rather go with strpos(), it would be closer to a 50% increase, but we lose the robustness of case-insensitive matching.
#4
i have never ever see http capitalized. i don't think we need to support that.
#5
I'm fine with strpos(), especially since it cuts the execution time almost by half, and I have attached a patch doing so.
Just for discussion though: this quote is from Internet Standard STD66, section 3.1, regarding URI schemes:
#6
thats better. thx.
#7
Do I need to do anything else at this point, or is this just waiting on Dries's review for committing?
#8
No test case?
#9
Argh, called me out on it, Dries! Ok... ok.... attached is a new patch that also includes some fixes for spaces in-between attributes in the generated html. I have also attached a test for the absolute image issue, as well as my new spacing fixes. This is the first test I have ever written, so I may be dead wrong in my approach :P
Just for performance sake, I tested this new patch by executing theme_image() 100 times. The average total increase after 5 rounds was approx. 2.09 milliseconds, which means each call to theme_image() was increased by 0.0209 milliseconds, which I believe is negligible. Is this ok?
#10
#11
Hmm, now that I think about it, this shouldn't check for "http" because that's too ambiguous. What if there is an internal link like "httpstuffiscool"?
#12
how about simply checking for http:// or https:// on the first positions to determine for internal links?
#13
Here's a new patch. I don't really like having two potential strpos checks, so if you can think of a better way, I am definitely all ears.
#14