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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

eMPee584’s picture

Status: Needs work » Needs review

WTF???? how could it fail? Huh?

Status: Needs review » Needs work

The last submitted patch failed testing.

eMPee584’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

Alright, it works when the leading a/b is stripped off.. tests pass.

attiks’s picture

Looks good to me

c960657’s picture

Status: Needs review » Needs work

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

eMPee584’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

code says

// In the previews, timestamps are added to prevent caching of images.

a few lines before..
i think moving the .'?'.time() into the braces might fix the issue, could you please try aswell?

Status: Needs review » Needs work

The last submitted patch failed testing.

eMPee584’s picture

Status: Needs work » Needs review
FileSize
2.29 KB

Rerolled 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).

eMPee584’s picture

Title: when editing image style, link to sample image broken without clean urls » when editing image style, link to sample image broken without clean urls (missing file_create_url())
eojthebrave’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.29 KB

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

Dries’s picture

+++ modules/image/image.admin.inc
@@ -779,21 +779,19 @@ function theme_image_style_preview($variables) {
   $output .= check_plain($style['name']) . ' (' . l(t('view actual size'), file_create_url($preview_file) . '?' . time()) . ')';

Not really part of this patch but it is better to use REQUEST_TIME instead of time().

Dries’s picture

Mmm, 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.

eMPee584’s picture

Status: Reviewed & tested by the community » Needs work

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

drewish’s picture

Status: Needs work » Needs review
FileSize
3.26 KB

I'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.

grendzy’s picture

Title: when editing image style, link to sample image broken without clean urls (missing file_create_url()) » when editing image style, link to sample image broken (missing file_create_url())

Even 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!

brianmercer’s picture

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

webchick’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

We should add a test for this.

brianmercer’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Sorry, didn't know that retest would change the status.

Status: Needs review » Needs work
Issue tags: +Needs tests

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

webchick’s picture

Issue tags: +beta blocker

This is a pretty nasty user-facing problem, so tagging as a beta blocker.

drewish’s picture

Status: Needs work » Needs review
FileSize
3.26 KB

Trying a re-roll.

Status: Needs review » Needs work

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

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Lets try this. Pretty much the same as the last one.

catch’s picture

Priority: Normal » Major

If this really blocks a beta, is it at least major.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm that this fixes the bug. Code looks good.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, 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.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -beta blocker

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