Posted by beejeebus on July 12, 2010 at 4:56pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | image.module |
| Category: | task |
| Priority: | critical |
| Assigned: | floretan |
| Status: | closed (fixed) |
Issue Summary
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.
Comments
#1
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.
#2
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.
#3
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.
#4
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.
#5
The last submitted patch, image.styles.patch, failed testing.
#6
alrighty, good news is that the only tests that break are related to this code.
#7
subscribing
#8
We need extended test to check how it works with locale modules enabled and language path prefix
#9
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.
#10
@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.
#11
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.#12
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.
#13
@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.
#14
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.
#15
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.
#16
huh. i realized looking again that my suggestion can't possibly be right; i was thinking that count() would return 0 for 'files'...
#17
The last submitted patch, image.styles.851878.15.patch, failed testing.
#18
catch makes a compelling argument for me too. lets go with #4 approach IMO. hopefully justin has time to finish up the patch.
#19
just a note to say i'll be back to this shortly.
#20
justin, substr_count() will help you simplifying that UGH menu definition.
#21
updated patch, image tests pass locally.
this is a simple follow on from #4.
#22
The last submitted patch, image.styles.patch, failed testing.
#23
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?
#24
The last submitted patch, image.styles.patch, failed testing.
#25
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;
}
/**
#26
The last submitted patch, image.styles_2.patch, failed testing.
#27
"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] ... hoping justindrendell can get back to this one.
#28
bump. no activity for a week. anyone? justinrandell?
#29
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.
#30
The last submitted patch, 851878.29.image-styles.patch, failed testing.
#31
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.
#32
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.
#33
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.
#34
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.
#35
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.
#36
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.
#37
Going to try to get this done for today's code sprint.
#38
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.
#39
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.
#40
Updated text to say "bypass PHP" instead of "bypass the menu system".
#41
I reviewed code and we are finally RTBC. Wait for green before commit.
#42
I don't understand why we return X-Image-Owned-By in the tests. Let's investigate that some more. Thanks!
#43
#44
Added an assertion in the test that actually checks if the custom 'X-Image-Owned-By' header has been set for private files.
#45
Back to RTBC.
#46
Great. Committed to CVS HEAD. Thanks.
#47
This apparently broke image derivatives for private images.
http://qa.drupal.org/head-status
#48
Actually, there were new files in the patch that were not committed properly.
Rerolled with just those.
#49
Committed those to HEAD, too. :) Thanks!
#50
Added the missing files to CVS. Run testbots, run!
#51
Automatically closed -- issue fixed for 2 weeks with no activity.