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.
Comment | File | Size | Author |
---|---|---|---|
#48 | 851878-image-styles-missing-files.patch | 1.31 KB | Damien Tournoud |
#44 | image_styles-851878-44.patch | 16.13 KB | floretan |
#40 | image_styles-851878-40.patch | 15.81 KB | floretan |
#39 | image_styles-851878-39.patch | 15.82 KB | floretan |
#33 | 851878.33.image-styles.patch | 14.4 KB | ksenzee |
Comments
Comment #1
catchsubscribing.
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.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedIMO, 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.
Comment #3
catchBumping 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.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedthis 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.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedalrighty, good news is that the only tests that break are related to this code.
Comment #7
jurgenhaassubscribing
Comment #8
andypostWe need extended test to check how it works with locale modules enabled and language path prefix
Comment #9
sunum. 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.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #11
aaron CreditAttribution: aaron commentedwouldn'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.Comment #12
aaron CreditAttribution: aaron commentedi 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.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #14
catchThe 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.
Comment #15
aaron CreditAttribution: aaron commentedhere'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.
Comment #16
aaron CreditAttribution: aaron commentedhuh. i realized looking again that my suggestion can't possibly be right; i was thinking that count() would return 0 for 'files'...
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedcatch makes a compelling argument for me too. lets go with #4 approach IMO. hopefully justin has time to finish up the patch.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedjust a note to say i'll be back to this shortly.
Comment #20
chx CreditAttribution: chx commentedjustin, substr_count() will help you simplifying that UGH menu definition.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch, image tests pass locally.
this is a simple follow on from #4.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedoh 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?
Comment #25
litwol CreditAttribution: litwol commentedOnly slightly modified patch.
Diff from previous version is:
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commented"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.
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commentedbump. no activity for a week. anyone? justinrandell?
Comment #29
ksenzeeThe 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.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedLooks 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.
Comment #32
ksenzeeIt 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.
Comment #33
ksenzeeThe 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.
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commentedThis 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.
Comment #35
moshe weitzman CreditAttribution: moshe weitzman commentedTested 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.
Comment #36
ksenzeeI 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.
Comment #37
floretan CreditAttribution: floretan commentedGoing to try to get this done for today's code sprint.
Comment #38
floretan CreditAttribution: floretan commentedOk, 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.
Comment #39
floretan CreditAttribution: floretan commentedHere'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.
Comment #40
floretan CreditAttribution: floretan commentedUpdated text to say "bypass PHP" instead of "bypass the menu system".
Comment #41
moshe weitzman CreditAttribution: moshe weitzman commentedI reviewed code and we are finally RTBC. Wait for green before commit.
Comment #42
Dries CreditAttribution: Dries commentedI don't understand why we return X-Image-Owned-By in the tests. Let's investigate that some more. Thanks!
Comment #43
Dries CreditAttribution: Dries commentedComment #44
floretan CreditAttribution: floretan commentedAdded an assertion in the test that actually checks if the custom 'X-Image-Owned-By' header has been set for private files.
Comment #45
moshe weitzman CreditAttribution: moshe weitzman commentedBack to RTBC.
Comment #46
Dries CreditAttribution: Dries commentedGreat. Committed to CVS HEAD. Thanks.
Comment #47
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis apparently broke image derivatives for private images.
http://qa.drupal.org/head-status
Comment #48
Damien Tournoud CreditAttribution: Damien Tournoud commentedActually, there were new files in the patch that were not committed properly.
Rerolled with just those.
Comment #49
webchickCommitted those to HEAD, too. :) Thanks!
Comment #50
Dries CreditAttribution: Dries commentedAdded the missing files to CVS. Run testbots, run!