Several things are not clear to me about theme_image_style(), opening this so we can either explain them better, or fix them.

http://api.drupal.org/api/drupal/modules--image--image.module/function/t...

Why do we do a file_exists() check here? image_style_url() will generate the image if it doesn't exist anyway:

"The absolute URL where a style image can be downloaded, suitable for use in an Only local images are allowed. tag. Requesting the URL will cause the image to be created"

http://api.drupal.org/api/drupal/modules--image--image.module/function/i...

This is doing a second and third file_exists() check (one that actually makes some sense since we don't want to generate images that already exist) - it makes much more sense to do this in the callback that generates the image than the one that's just generating a link.

CommentFileSizeAuthor
#22 remove-crap-from-image-style-functions.patch3.79 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 31,517 pass(es). View
#21 remove-crap-from-image-style-functions.patch3.8 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 31,547 pass(es). View
#19 remove-crap-from-image-style-functions.patch1.78 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 31,528 pass(es). View
#11 remove-crap-from-image-style-functions.patch1.49 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 31,535 pass(es). View
#10 theme_image_style_1025124.patch3.95 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 31,510 pass(es). View
#8 theme_image_style_1025124.patch3.44 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 31,524 pass(es), 1 fail(s), and 0 exception(es). View
#1 theme_image_style_1025124.patch599 bytescatch
FAILED: [[SimpleTest]]: [MySQL] 31,508 pass(es), 5 fail(s), and 0 exception(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

catch’s picture

Title: theme_image_style() is very confusing » Remove cruft from theme_image_style()
Status: Active » Needs review
Issue tags: +Performance, +i/o
FileSize
599 bytes
FAILED: [[SimpleTest]]: [MySQL] 31,508 pass(es), 5 fail(s), and 0 exception(es). View

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.

Status: Needs review » Needs work

The last submitted patch, theme_image_style_1025124.patch, failed testing.

catch’s picture

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.

jbrown’s picture

catch’s picture

Status: Needs work » Needs review

Retesting, just in case.

catch’s picture

@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.

Status: Needs review » Needs work

The last submitted patch, theme_image_style_1025124.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
FAILED: [[SimpleTest]]: [MySQL] 31,524 pass(es), 1 fail(s), and 0 exception(es). View

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

Status: Needs review » Needs work

The last submitted patch, theme_image_style_1025124.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
3.95 KB
PASSED: [[SimpleTest]]: [MySQL] 31,510 pass(es). View

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.

jbrown’s picture

Priority: Major » Normal
FileSize
1.49 KB
PASSED: [[SimpleTest]]: [MySQL] 31,535 pass(es). View

I think the problem is that image_style_url() is a pile of crap.

jbrown’s picture

Title: Remove cruft from theme_image_style() » Remove cruft from theme_image_style() and image_style_url()

image_style_url() is essentially a duplication of code from these functions:

DrupalPrivateStreamWrapper::getExternalUrl()
image_style_path()
file_create_url()

When the cruft is removed from theme_image_style(), image_style_url() borks up the tests.

Cleanly re-implementing image_style_url() to use the correct API functions means that the tests no longer fail - so presumably image_style_url() is currently buggy (in addition to being duplicated code).

catch’s picture

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.

jbrown’s picture

With manual testing every combination of public/private files and clean/unclean urls works correctly.

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.

catch’s picture

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.

jbrown’s picture

Status: Needs work » Needs review

Actually - with my patch image generation does not work [edit: for public stream wrapper] when mod_rewrite has been disabled. Disabling clean urls is not enough to trigger this bug.

Will look at this further.

jbrown’s picture

Status: Needs review » Needs work
jbrown’s picture

Status: Needs review » Needs work

The problem is that if mod_rewrite is not enabled, then it is not possible for the generation url and the url of the generated file to be the same. [edit: for public stream wrapper]

the file_exists() call is essential if clean urls are not enabled.

I will update my patch to do this.

jbrown’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
PASSED: [[SimpleTest]]: [MySQL] 31,528 pass(es). View

Okay - I think this might be the correct solution.

catch’s picture

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.

catch’s picture

FileSize
3.8 KB
PASSED: [[SimpleTest]]: [MySQL] 31,547 pass(es). View

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.

jbrown’s picture

FileSize
3.79 KB
PASSED: [[SimpleTest]]: [MySQL] 31,517 pass(es). View

Great!

There is a typo in your test.

variable_set('file_default_schema', $clean_url);

should be

variable_set('clean_url', $clean_url);

catch’s picture

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.

jbrown’s picture

Status: Needs review » Reviewed & tested by the community

#21 passed because variable clean_url was defaulting to FALSE.

I tested the test by breaking the code and it detected it.

This is good to go!

catch’s picture

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Should be a nice performance improvement on some file systems.

Status: Fixed » Closed (fixed)

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

YesCT’s picture

Issue tags: -i/o +i-o

(the slash in the i/o tag breaks the autocomplete from adding new tags)