Example of two image tags:
<img src="/wiris/showimage?formula=2313f2f42d6f65e77a7321ba14da2640.png">
<img src="/wiris/showimage.png?formula=2313f2f42d6f65e77a7321ba14da2640">

These two are translated to:
<img src="/wiris/showimage">
<img src="/wiris/showimage.png">
The query string is missing.

In the file cdn.fallback.inc, function cdn_html_alter_image_urls there are two lines like this:
_cdn_html_alter_file_url($html, $pattern, 0, 4, 5, 1, 7);
If I alter the line to this (I have only tried replacing the second line):
_cdn_html_alter_file_url($html, $pattern, 0, 4, 6, 1, 7);
it all begin working again.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jamesharv’s picture

Version: 6.x-2.5 » 7.x-2.5
Status: Active » Needs review
FileSize
1.14 KB
1.14 KB

I've had this same problem with 7.x-2.5. Changing the $querystring_index argument from 5 to 6 solved it for me as well, so I've created and attached a patch for both 6.x-2.x and 7.x-2.x.

I haven't tested the 6.x-2.x patch. Use at own risk.

I have tested the 7.x-2.x patch in my scenario only. If you use it, you will want to carefully check that it doesn't break any of your site's HTML.

Status: Needs review » Needs work

The last submitted patch, wrong-preg-index-for-query-string-7.x-2.x-1864536-1.patch, failed testing.

jamesharv’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

FYI, the above 7.x-2.x patch doesn't apply cleanly to 7.x-2.5. So here is the 7.x-2.5 patch I'm using, in case it's useful to anyone.

mcrittenden’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm the issue and the patch in #3 fixes it for me.

greggles’s picture

Priority: Major » Critical

This is a really really annoying bug. You can debug your html all day trying to figure out where it's getting stripped and then....it's cdn module. It took me doing a git bisect to figure it out and seeing the addition of the cdn module as the cause.

I don't know the options of _cdn_html_alter_file_url well enough to know whether this is the exact right combination of parameters, but I do know that this was causing a problem and the patch seems to fix it.

I'm upgrading this to critical because it can really break a site and it is super duper hard to track down. IMO that deserves critical.

greggles’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
798 bytes

My problem actually sounds the same as #1832616: CDN Module improperly rewrites the HREF of <a> tags wrapping <img> tags. It's interesting that this patch fixes that.

This patch actually no longer applies to 7.x-2.x-dev. I re-rolled it, but I wonder if it's actually still necessary or not so marking back to needs review.

Status: Needs review » Needs work

The last submitted patch, wrong-preg-index-for-query-string-7.x-2.5-1864536-6.patch, failed testing.

greggles’s picture

Version: 7.x-2.5 » 7.x-2.x-dev
Status: Needs work » Needs review

Patch applies fine for me....hmmm.

greggles’s picture

Wim Leers’s picture

Title: Missing query string in image » Support dynamically generated images (via query strings)
Category: bug » feature
Priority: Critical » Minor
FileSize
4.71 KB

The CDN module intentionally/explicitly strips query strings. For the same reason as it doesn't support this IE font-face hack (#1514182-1: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation):

Actually, the CDN module explicitly *strips* query strings because they're bad for cacheability! I understand that it's useful in this particular case, but if anything, you should add this to cdn.basic.css.inc, and *only* allow query strings to exist if the result is a superstring of ".eot?#iefix".

(Several CDNs and proxies ignore query strings, hence no functionality is allowed to rely on this!)

Of course, I still need to figure out #1926884: CDN module is not compatible with security fix in Drupal core update 7.20, but that's been the rule; so in fact this was not a bug in the code (it could be marked a bug in the docs).


All the above being said, the use case outlined in the OP is of course valid: a script that generates images should work. However, the solution provided in the OP and later is faulty because it allows query strings to be present on all images, and that is wrong. We should discourage the use of query strings as much as possible.

Attached are unit tests to cover all this. They should be cleaned up, but they work. The behavior desired by the OP is just commented out, uncomment it and once you have all tests passing, this should be RTBC.

Of course, if you control the service generating these images, you could just as well employ URL rewriting to change the query string into a "regular" path (i.e. showimage?formula=123.png -> showimage/formula-123.png). Then you wouldn't have any problems.

Status: Needs review » Needs work

The last submitted patch, image_url_altering_tests-1864536-10.patch, failed testing.

Wim Leers’s picture

(As you can see, I included the patch of #6, but tests fail because of those changes.)

greggles’s picture

Here's some more details on why I care about this issue.

I have a block with content like this:

<li class="first"><a href="/james-dean?option=1" class="active"><img typeof="foaf:Image" src="https://cdn.example.com.com/Card-JamesDean-5.jpg" alt=""/></a></li>
<li><a href="/james-dean?option=3" class="active"><img typeof="foaf:Image" src="https://cdn.example.com.com/Card-JamesDean-15.jpg" alt=""/></a></li>
<li><a href="/benin?option=0"><img typeof="foaf:Image" src="https://cdn.example.com.com/flag-benin.jpg" alt=""/></a></li>

When I enable the CDN module without this patch the content turns into this:

<li class="first"><a href="/james-dean" class="active"><img typeof="foaf:Image" src="https://cdn.example.com.com/Card-JamesDean-5.jpg" alt=""/></a></li>
<li><a href="/james-dean" class="active"><img typeof="foaf:Image" src="https://cdn.example.com.com/Card-JamesDean-15.jpg" alt=""/></a></li>
<li><a href="/benin"><img typeof="foaf:Image" src="https://cdn.example.com.com/flag-benin.jpg" alt=""/></a></li>

Note how the href element is losing it's query parameter which breaks my site. I don't see how that can be anything but a bug at some priority level higher than minor. Maybe it's a coincidence that this patch fixes my problem and my problem should really be fixed in some other way.

mcrittenden’s picture

Priority: Minor » Normal

I think this should be at the very least be normal priority, as it is a "Bug that affects one piece of functionality" (http://drupal.org/node/45111).

However, the solution provided in the OP and later is faulty because it allows query strings to be present on all images, and that is wrong. We should discourage the use of query strings as much as possible.

I don't think a blanket rule to remove them is a good way to go about this. This breaks things more than it helps things.

#13 is the same issue I had. I also ran into the issue of stripping from image src'es. In my use case I am using img src="/inc.php?nid=12345" to increment a node counter without resorting to AJAX (I'm using Varnish so hook_node_view won't work). The inc.php script just outputs a 1x1 empty GIF.

greggles’s picture

It's maybe worth noting that the latest core release and upcoming imagecache 6.x release added querystring parameters to images to prevent denial of service.

jamesharv’s picture

I personally don't think CDN should be stripping query parameters from URLs. I agree that they are bad for cacheability, but sometimes they are necessary. It should be the responsibility of a site developer to remove these if possible.

FYI the feature this broke for me was a feed icon attached to a view. The feed icon image didn't have any query parameters, but it was wrapped by an anchor tag which did have query parameters in the href. These were getting stripped. Eg.

<a href="view-url?name=test&category=1"><img src="feed-icon.png" /></a>

Became

<a href="view-url"><img src="feed-icon.png" /></a>

It was very hard to figure out why the feed wasn't inheriting the view's filters!

Wim Leers’s picture

#13: AFAICT that (i.e. your particular use case) was fixed in the dev snapshot. The <a> URL does not end in ".jpg" like the <img> URL does, hence the CDN module won't modify it.

#14:
Hah, I had no idea such an official classification existed!

I don't think a blanket rule to remove them is a good way to go about this. This breaks things more than it helps things.

Sure; that's why I reach out to you by offering tests. Make the tests pass and I'll commit it :)

#13 is the same issue I had.

In that case, see my answer to #13.

#15: I already anticipated this remark; reply is already at #10:

Of course, I still need to figure out #1926884: CDN module is not compatible with security fix in Drupal core update 7.20, but that's been the rule; so in fact this was not a bug in the code (it could be marked a bug in the docs).

#16:
Again, see my answer to #13.

greggles’s picture

I appreciate your perspective that the dev should have fixed this. It did not.

Wim Leers’s picture

(Wrapped HTML tags in "code tags" in #17, to fix HTML breakage.)

#18: Interestingly, the tests say otherwise… So it seems the example you put forward in #13 deviates from what is happening exactly on your site. Please note that for the patch in #10, only two tests fail: the two "Image HTML correctly altered (query string stripped)." cases. The "Linked image HTML correctly altered (anchor unmodified, even with query strings)." is the one that matches your example in #13 and that one passes just fine. So… could you please reroll the patch to include your *precise* use case as a test case?

greggles’s picture

Sadly I don't have time for that.

Wim Leers’s picture

Well, I wrote tests that indicate what you're saying is not (or no longer — even though the code base hasn't changed at all in that aspect) true. I'm not sure how I could possibly do more for you — if I can't reproduce it, then I can't fix it :) Are you sure that #13 is *precisely* what is happening? Could you share an unmodified example — unmodified besides having the domain names replaced with example.com?

greggles’s picture

If you think it's not a bug that's fine, do what you want to do in this release. If it's still a bug I'll patch that release and report back.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.71 KB

I just committed the tests at #10 after refactoring them at #1929918: Test coverage for image URL rewriting.

This has allowed the patch of #10 to become a whole lot easier, and now there's only one test that fails. In order to make this feature request happen, you just need to make sure that single test passes.

Wim Leers’s picture

My deity, am I a retard or what? I simply reuploaded the #10 patch, stupid! Here's the right one.

Status: Needs review » Needs work

The last submitted patch, image_url_altering_tests-1864536-23.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Postponed

As you can see, only one test failed in #24. Make that test pass (and of course keep the others passing as well) and this can become committable.

Postponing because I'm not going to work on this, and it doesn't look like anybody is going to any time soon.

Wim Leers’s picture

Status: Postponed » Closed (fixed)

To achieve Drupal >=7.20 compatibility, I was forced to allow this.

Hence, this is now supported as per #1926884-8: CDN module is not compatible with security fix in Drupal core update 7.20.

Wim Leers’s picture

Issue summary: View changes

Missing the code tag