Remove cruft from theme_image_style() and image_style_url()

So I looked back through #851878: serve image derivatives from the same url they are generated from which was the last major change here, and that didn't change theme_image_style() at all. The last change to theme_image_style() was in 2009, when the "change the image path depending on whether or not the file exists or not" method was still in use - where these would be completely different paths being used.

Not only that, but whether or not the file exists, once it's been passed to theme_image(), we do this:

$attributes['src'] = file_create_url($variables['path']);

And regardless of which version it gets passed, the results of that are always the same - this counts for both public and private files.

So as far as I can see, this is just an oversight that was left in when we worked on #851878: serve image derivatives from the same url they are generated from, and we should use image_style_path() all the time.

Leaving this as major, since the file_exists() here with a few images displayed, over NFS is very expensive - up to 7% of the page request with only a handful of images.

Did manual testing but didn't run simpletests on this yet.

I can't reproduce those test failures locally - all the image module tests pass with the patch applied, but when doing so I ran into #1042150: Upload user pictures fails on (IIS) installs with c:\windows\temp as upload_tmp_dir. with a clean HEAD.

Retesting, just in case.

@jbrown: I saw that comment by Damien, but I think that might be referring to the old design. Haven't caught Damien on irc to check if that's what he meant though.

Still can't reproduce these locally, looking at the tests, wonder if this would do it.

Better...

edit: so the issue was that tests were using image_style_url() and assertRaw() to check the functionality, but since we changed that to always use image_style_path() that's not longer valid. I'm not sure why it continued working on my localhost and failed on the test bot, but the test changes make sense in their own right I think.

This is much nicer. I didn't manage to check properly locally yet, and there's no test for it that I can see - does this still work without clean urls? Will check tomorrow if no-one else does.

The image module has extensive tests for public/private files.

The private stream wrapper has tests to ensure it works with clean/unclean urls.

DrupalPrivateStreamWrapper::getExternalUrl() calls url() so it works for both clean and unclean urls.

Unless I'm reading it wrong we don't have anything that covers public stream wrapper and unclean urls then?

When I did a quick test earlier it worked, but I was convinced that I needed to go a bit further to break it.

In #851878: serve image derivatives from the same url they are generated from we wanted everything served from the same url all the time, so that it wouldn't interfere with page caching, the trade-off made there was to always serve images via PHP on sites that don't have clean urls enabled.

This works out as a big improvement for the non-clean URLs case, since while you still might get a page or block that caches the ?q= version, that's what happens in HEAD for every request now anyway, and the file_exists() is better than the full bootstrap you'd get.

I think the patch is functionally RTBC, but:

+  if (!variable_get('clean_url') && file_uri_scheme($uri) == 'public' && !file_exists($uri)) {
+    $directory_path = file_stream_wrapper_get_instance_by_uri($uri)->getDirectoryPath();
+    return url($directory_path . '/' . file_uri_target($uri), array('absolute' => TRUE));
   }

This needs comments to explain what's going on.

We should also write a test for the public + non-clean urls case so that the behaviour is enforced. We can't disable mod_rewrite for the test, so probably it needs to check for ?q in the image path to actually fail if this got broken.

Will hopefully be able to look at these a bit later today.

Here's a patch with additional comments and modifications to the test to include clean and non-clean URLs. Completely untested but have to go out now so posting so the test bot can have a look.

Argh, that's what I get for rolling patches on the way out, but now I wonder why the tests passed in both #21 and #22.

Agreed on RTBC. While it's just us two in the issue, I've reviewed jbrown's code, and jbrown's reviewed my test.