Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
there's some fluke that prevented me from activating clean urls up to now, turns out that exposed a little glitch in the image module where the sample image file path (modules/image/sample.png) is not passed through file_create_url which results in a q= being appended in front of it - b0rk. Patch fixes the link by doing that.
Comment | File | Size | Author |
---|---|---|---|
#24 | image-broken_sample_image-563382-24-eojthebrave.patch | 2.95 KB | eojthebrave |
#22 | image_563382.patch | 3.26 KB | drewish |
#15 | image_563382.patch | 3.26 KB | drewish |
#11 | 563382-11-eojthebrave-image_fix_previx_link.patch | 2.29 KB | eojthebrave |
#9 | image-fix-sample-image-link-iii.patch | 2.29 KB | eMPee584 |
Comments
Comment #2
eMPee584 CreditAttribution: eMPee584 commentedWTF???? how could it fail? Huh?
Comment #4
eMPee584 CreditAttribution: eMPee584 commentedAlright, it works when the leading a/b is stripped off.. tests pass.
Comment #5
attiks CreditAttribution: attiks commentedLooks good to me
Comment #6
c960657 CreditAttribution: c960657 commentedThis does not work if the
image_style_preview_image
variable is "public://foo.jpg" (the existing code doesn't either).It looks like adding
'?' . time()
is problematic. You can use url() to modify an absolute URLs. But I don't understand why it is necessary to add this to the original image in the first place.Comment #7
eMPee584 CreditAttribution: eMPee584 commentedcode says
a few lines before..
i think moving the
.'?'.time()
into the braces might fix the issue, could you please try aswell?Comment #9
eMPee584 CreditAttribution: eMPee584 commentedRerolled the patch after the changes to the theme() functions. Also tested setting the image_style_preview_image var to a streamwrapper URI, which now works. Giving it some more thought, the timestamps appended to prevent image caching are really unnecessary for the original sample image, as that is not supposed to change (although theoretically one could set image_style_preview_image to point to f.e. a dynamically generated image).
Comment #10
eMPee584 CreditAttribution: eMPee584 commentedComment #11
eojthebraveTested patch in #9 and confirmed that it works. Code looks good to me. file_create_url() does indeed handle stream wrapper schemes so no problems there.
Steps to repeat the problem.
1. Turn clean urls off.
2. Create an image style
3. Navigate to page for editing new image style and click the 'view actual size' link for one of the two preview images.
This should result in a 404.
Apply the patch and presto, no more 404.
The attached patch is just a simple re-roll of #9 to clean up some offset issues when applying. No other changes.
Comment #12
Dries CreditAttribution: Dries commentedNot really part of this patch but it is better to use REQUEST_TIME instead of time().
Comment #13
Dries CreditAttribution: Dries commentedMmm, I think
file_create_url($preview_file) . '?' . time()
could actually be buggy. file_create_url() can decide to the image from a CDN. Not sure what happens when you append time in that case.Comment #14
eMPee584 CreditAttribution: eMPee584 commented..and on the contrary, what would happen if the annexed time would be put inside the braces when retrieving from a CDN? Someone needs to test this.. although it is highly unlikely this will be relevant for this example image.
Comment #15
drewish CreditAttribution: drewish commentedI'll take a crack at this. Rather than calling file_create_url() a bunch of times I'm storing it into a variable. I also changed time() for REQUEST_TIME. I'm not sure how concatenating the time will work but I think for the majority of people it will be fine. To be a little safer I've changed it to a proper query parameter e.g. cache_bypass=1271719850.
Comment #16
grendzy CreditAttribution: grendzy commentedEven with clean urls enabled, the sample image seems busted (at least in Safari), because the ? in the query string gets encoded as %3F.
I haven't reviewed the code, but the patch did fix the issue for me. Thanks drewish!
Comment #17
brianmercer CreditAttribution: brianmercer commentedStill having the same issue as #16 with 7.x-dev (nightly snapshot 10/5/2010).
Page admin/config/media/image-styles/edit/large contains broken image: /modules/image/sample.png%3F1286309053.
Patch at #15 will not apply.
Comment #18
webchickWe should add a test for this.
Comment #19
brianmercer CreditAttribution: brianmercer commentedSorry, didn't know that retest would change the status.
Comment #21
webchickThis is a pretty nasty user-facing problem, so tagging as a beta blocker.
Comment #22
drewish CreditAttribution: drewish commentedTrying a re-roll.
Comment #24
eojthebraveLets try this. Pretty much the same as the last one.
Comment #25
catchIf this really blocks a beta, is it at least major.
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedI can confirm that this fixes the bug. Code looks good.
Comment #27
webchickOk, spoke to some folks on IRC and apparently it's not easy to write a test for this. So let's just fix the bug, at least for now.
Committed to HEAD.