Over in #827206: image_style_url() only prevents page caching some of use discovered that we don't generate and serve image derivatives at the same URL like ImageCache does.

The reason for this decision was to allow image.module to work without clean URL support, which still stands - this issue is not about changing that.

However, discussing this in #drupal with quicksketch and catch, we came to the conclusion that we can support sites without clean URLs by simply serving the image derivatives via PHP with links like <img src="/?q=image/generate/foo/bar.jpg" />.

This will simplify the code considerably, and won't impact most sites, as the vast majority use clean URLs.

Files: 
CommentFileSizeAuthor
#48 851878-image-styles-missing-files.patch1.31 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 851878-image-styles-missing-files.patch. View
#44 image_styles-851878-44.patch16.13 KBfloretan
PASSED: [[SimpleTest]]: [MySQL] 23,228 pass(es). View
#40 image_styles-851878-40.patch15.81 KBfloretan
PASSED: [[SimpleTest]]: [MySQL] 23,183 pass(es). View
#39 image_styles-851878-39.patch15.82 KBfloretan
PASSED: [[SimpleTest]]: [MySQL] 23,189 pass(es). View
#33 851878.33.image-styles.patch14.4 KBksenzee
PASSED: [[SimpleTest]]: [MySQL] 22,908 pass(es). View
#29 851878.29.image-styles.patch13.88 KBksenzee
FAILED: [[SimpleTest]]: [MySQL] 22,902 pass(es), 1 fail(s), and 0 exception(es). View
#25 image.styles_2.patch9.7 KBlitwol
FAILED: [[SimpleTest]]: [MySQL] 22,474 pass(es), 6 fail(s), and 1 exception(es). View
#23 image.styles.patch9.67 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 22,180 pass(es), 6 fail(s), and 1 exception(es). View
#21 image.styles.patch8.71 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 22,174 pass(es), 12 fail(s), and 4 exception(es). View
#15 image.styles.851878.15.patch4.31 KBaaron
FAILED: [[SimpleTest]]: [MySQL] 22,116 pass(es), 26 fail(s), and 4 exception(es). View
#4 image.styles.patch4.21 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 22,086 pass(es), 28 fail(s), and 4 exception(es). View

Comments

catch’s picture

subscribing.

I'd say due to the various complexities around the other issue, that should be postponed on this one, and this bumped to critical, but leave that up to Justin.

moshe weitzman’s picture

IMO, we should consider simply making image styles dependant on rewrite capability. Rewrite is now on apache, nginx, and iis. At some point we are wasting engineering resources by catering to fringes.

catch’s picture

Priority: Normal » Critical

Bumping this to critical. At the moment any module doing html caching (block, views, panels etc.) will cache image paths leading to 403s if the cache miss is also an image derivative miss. quicksketch suggested using drupal_page_is_cacheable() in irc, this is not an option for authenticated caching because that could very well end up being set to false on each request.

I'd be fine on not having image styles without clean urls too, but justin's idea seems like it'd be relatively simple to implement - certainly easier than fixing the caching issues in current HEAD.

beejeebus’s picture

Status: Active » Needs review
FileSize
4.21 KB
FAILED: [[SimpleTest]]: [MySQL] 22,086 pass(es), 28 fail(s), and 4 exception(es). View

this still needs work, but i want to see how many tests it breaks.

the functionality seems to be working, but i'm sure the code is not the cleanest, and i'm not 100% sure the changes to image_style_url() are up to scratch.

Status: Needs review » Needs work

The last submitted patch, image.styles.patch, failed testing.

beejeebus’s picture

alrighty, good news is that the only tests that break are related to this code.

jurgenhaas’s picture

subscribing

andypost’s picture

We need extended test to check how it works with locale modules enabled and language path prefix

sun’s picture

um. If we dropped the one-single-URL-to-generate-and-view approach for D7 for some reason, we better find out that reason before trying to drop the reason for dropping the reason?

In the case it matters, I'm with catch, "ImageCache" only needs to work with clean URLs. If that was the only reason to drop aforementioned, original approach of ImageCache, then it was wrong to change it.

moshe weitzman’s picture

@sun - we already got a report from quicksketch (the principal author) that the double urls were introduced to accomodate non clean urls. see the opening post here.

sun, catch, and moshe all agree that dropping support for non-clean is the best way forward.

aaron’s picture

wouldn't 'page arguments' => array(count(explode('/', file_directory_path())) + 1), actually need to be +2? i think that would take care of it; i'll try to test when i'm on my desktop later today.

aaron’s picture

sun, catch, and moshe all agree that dropping support for non-clean is the best way forward.

i think that if we do that, we need to do that across the board. otherwise, if drupal continues to support non-clean, then all of core needs to continue to support non-clean, IMHO.

moshe weitzman’s picture

@aaron - this black or white position is not helpful IMO. there are lots of features in core that have dependencies. and when you are missing a dependency, your feature does not work. image api itself depends on an image toolkit, for example. we are proposing that image styles depend on clean-urls, as it has for the life of imagecache. check out the status report for more examples.

catch’s picture

The approach in #4 actually works with non-clean urls in the sense that images will be displayed - it just doesn't do the cache bit of imagecache in that the generation path will be hit every time. It's completely reasonable to expect production sites which care about performance in the slightest to have clean urls enabled, so I don't see any problem with that approach at all - and the image derivatives themselves will continue to look the same.

aaron’s picture

Status: Needs work » Needs review
FileSize
4.31 KB
FAILED: [[SimpleTest]]: [MySQL] 22,116 pass(es), 26 fail(s), and 4 exception(es). View

here's a re-roll with my suggestion. i'll leave it to everyone else to duke out the merits, as i'm neutral to the outcome, though catch makes a good case.

aaron’s picture

huh. i realized looking again that my suggestion can't possibly be right; i was thinking that count() would return 0 for 'files'...

Status: Needs review » Needs work

The last submitted patch, image.styles.851878.15.patch, failed testing.

moshe weitzman’s picture

catch makes a compelling argument for me too. lets go with #4 approach IMO. hopefully justin has time to finish up the patch.

beejeebus’s picture

just a note to say i'll be back to this shortly.

chx’s picture

justin, substr_count() will help you simplifying that UGH menu definition.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
8.71 KB
FAILED: [[SimpleTest]]: [MySQL] 22,174 pass(es), 12 fail(s), and 4 exception(es). View

updated patch, image tests pass locally.

this is a simple follow on from #4.

Status: Needs review » Needs work

The last submitted patch, image.styles.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
FAILED: [[SimpleTest]]: [MySQL] 22,180 pass(es), 6 fail(s), and 1 exception(es). View

oh dear. take two. there are still some fails when testing the 'private' scheme, but i'm at a bit of loss about how to treat that right now.

looking at the current code, we don't seem to do any access checks (in php) on derivatives of private files?

Status: Needs review » Needs work

The last submitted patch, image.styles.patch, failed testing.

litwol’s picture

Status: Needs work » Needs review
FileSize
9.7 KB
FAILED: [[SimpleTest]]: [MySQL] 22,474 pass(es), 6 fail(s), and 1 exception(es). View

Only slightly modified patch.

Diff from previous version is:

=== modified file 'modules/image/image.module'
--- modules/image/image.module  2010-07-24 13:35:47 +0000
+++ modules/image/image.module  2010-08-02 03:28:51 +0000
@@ -767,7 +767,7 @@
   if (!variable_get('clean_url', FALSE)) {
     $url = '?q=' . $url;
   }
-  return $url;
+  return base_path() . $url;
 }
 
 /**

Status: Needs review » Needs work

The last submitted patch, image.styles_2.patch, failed testing.

moshe weitzman’s picture

"we don't seem to do any access checks (in php) on derivatives of private files?". that sounds like a critical issue to me. could be fixed separately, and perhaps by #846296: file_file_download() only implements access checks for nodes and users ... hoping justindrendell can get back to this one.

moshe weitzman’s picture

bump. no activity for a week. anyone? justinrandell?

ksenzee’s picture

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

The attached patch should make private file derivatives work correctly. It adds a system/files/styles/% callback that both generates new derivatives and delivers old ones, with access checks. It also adds a test module that implements hook_file_download for test images. I think all my changes to the tests are reasonable but I've been staring at it all day and could have missed something.

Before commit, or as a followup, we should move image.test from modules/image to modules/image/tests. I didn't want to do that here since it would make the test changes impossible to review.

Status: Needs review » Needs work

The last submitted patch, 851878.29.image-styles.patch, failed testing.

moshe weitzman’s picture

Looks like this adds access control on derivitives but does not address "serve image derivatives from the same url they are generated from". @ksenzee - do you plan to tackle the rest?

1 test failure in last patch.

ksenzee’s picture

It should serve image derivatives from the same url they are generated from. It's a different URL for public derivatives than for private derivatives, but it's the same URL for generating and serving. I'm looking at the test failure now.

ksenzee’s picture

Status: Needs work » Needs review
FileSize
14.4 KB
PASSED: [[SimpleTest]]: [MySQL] 22,908 pass(es). View

The test failure was a test using image_style_url(), which is supposed to return an absolute URL, when what it wanted was a path to feed into theme_image.

moshe weitzman’s picture

This is looking good. I will study it some more soon. Meanwhile, I'm still a bit troubled by the page callback names. The patch shows one called 'image_style_generate' and another 'image_style_deliver'. But both of them are capable of generating and delivering as needed. I might go for image_deliver() and image_deliver_private(). I think the surrounding code comments could also be reviewed for clarity.

moshe weitzman’s picture

Status: Needs review » Needs work

Tested this out and it works well.

In addition to my comments in #34, I'm wondering if we can improve image_style_deliver(). Does it have to do that weirdness with func_get_args()? Just would be nice if that disappeared but no big deal. Also, I would happier if the access control for private files was an 'access callback' and not buried in the page callback.

ksenzee’s picture

I would happier if the access control for private files was an 'access callback' and not buried in the page callback.

I would too, but it seemed wisest to handle it in the same way as file_download handles all other private file downloads, which is with an access callback of TRUE and access checking in the page callback. Maybe something to look at for D8.

IIRC image_style_deliver (or image_deliver_private, which is a better name) does have to do func_get_args() weirdness. I'll look at it again though and see if that's true.

floretan’s picture

Assigned: Unassigned » floretan

Going to try to get this done for today's code sprint.

floretan’s picture

Ok, first a quick summary of what this patch does:

How it worked before:
If the image derivative doesn't exist, the image URL is set to a menu callback that generates the image. When the image derivative exists, the image URL is set directly to the file using file_create_url(). The problem is that we have two separate URLs.

How it works with this patch:
1) Public URL: The menu callback is set to handle the same URL as the one matching the image derivative. If using clean URLs and the image derivative exists, files are served directly (bypassing the menu system). In all other cases the image is served through the menu system.
2) Private URL: Image derivatives are always served through the menu system.

In all cases, the URL of an image derivative is the same when generating the derivative for the first time or when delivering an existing one.

Minor details adjusted by this patch:
* Removes the check that image derivatives are only created within a certain time period after the image tag referencing the derivative were generated. Imagecache has never done this, and it's not clear that there are any real benefits from doing this. I discussed this "feature" with Moshe to make sure it's ok to get rid of it.
* Remove the call to file_create_url() before passing parameters to theme('image'). theme_image() calls it internally anyways, so this just simplifies the code without changing the behavior at all.

floretan’s picture

Status: Needs work » Needs review
FileSize
15.82 KB
PASSED: [[SimpleTest]]: [MySQL] 23,189 pass(es). View

Here's an updated patch. We still have two separate menu items for public and private files, but they use the same callback to generate/deliver the images, reducing the code duplication. The two first parameters to the menu callback ($style and $scheme) are now defined explicitly. More comments have also been added to explain how things are really working.

floretan’s picture

FileSize
15.81 KB
PASSED: [[SimpleTest]]: [MySQL] 23,183 pass(es). View

Updated text to say "bypass PHP" instead of "bypass the menu system".

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed code and we are finally RTBC. Wait for green before commit.

Dries’s picture

Status: Reviewed & tested by the community » Needs review

I don't understand why we return X-Image-Owned-By in the tests. Let's investigate that some more. Thanks!

Dries’s picture

Status: Needs review » Needs work
floretan’s picture

Status: Needs work » Needs review
FileSize
16.13 KB
PASSED: [[SimpleTest]]: [MySQL] 23,228 pass(es). View

Added an assertion in the test that actually checks if the custom 'X-Image-Owned-By' header has been set for private files.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Great. Committed to CVS HEAD. Thanks.

Damien Tournoud’s picture

Status: Fixed » Needs work

This apparently broke image derivatives for private images.

http://qa.drupal.org/head-status

Damien Tournoud’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 851878-image-styles-missing-files.patch. View

Actually, there were new files in the patch that were not committed properly.

Rerolled with just those.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed those to HEAD, too. :) Thanks!

Dries’s picture

Added the missing files to CVS. Run testbots, run!

Status: Fixed » Closed (fixed)

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