theme_image() should check for absolute urls

strpos() would be a bit faster and more readable, no? otherwise, nice work.

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.

i have never ever see http capitalized. i don't think we need to support that.

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:

Although schemes are case-insensitive, the canonical form is lowercase and documents that specify schemes must do so with lowercase letters. An implementation should accept uppercase letters as equivalent to lowercase in scheme names (e.g., allow "HTTP" as well as "http") for the sake of robustness but should only produce lowercase scheme names for consistency.

thats better. thx.

Do I need to do anything else at this point, or is this just waiting on Dries's review for committing?

No test case?

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?

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"?

how about simply checking for http:// or https:// on the first positions to determine for internal links?

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.

I'd like to get this in. Just spent a half hour trying to get theme_image() to work with private files. :P

Could we re-use menu_path_is_external()?

How about something like this instead?

This is at least a normal priority, and I'm going to change it to a bug report instead. theme_image() and file_create_url() should work nicely with one another.

Hmmm. #278770: file_create_url only returns absolute URL (containing the domain name) might be another approach.

Yeah. Marking this one a "won't fix" in favour of the other.

Solving the problem this way incurs a minute performance penalty on every call to theme('image'), even if people aren't using private files. The other patch has the same effect but only with those files you're specifically calling file_create_url() on.