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.
Comments
Comment #1
jamesharv CreditAttribution: jamesharv commentedI'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.
Comment #3
jamesharv CreditAttribution: jamesharv commentedFYI, 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.
Comment #4
mcrittenden CreditAttribution: mcrittenden commentedI can confirm the issue and the patch in #3 fixes it for me.
Comment #5
gregglesThis 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.
Comment #6
gregglesMy 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.
Comment #8
gregglesPatch applies fine for me....hmmm.
Comment #9
greggles#6: wrong-preg-index-for-query-string-7.x-2.5-1864536-6.patch queued for re-testing.
Comment #10
Wim LeersThe 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):
(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.Comment #12
Wim Leers(As you can see, I included the patch of #6, but tests fail because of those changes.)
Comment #13
gregglesHere's some more details on why I care about this issue.
I have a block with content like this:
When I enable the CDN module without this patch the content turns into this:
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.
Comment #14
mcrittenden CreditAttribution: mcrittenden commentedI 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).
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.
Comment #15
gregglesIt'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.
Comment #16
jamesharv CreditAttribution: jamesharv commentedI 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!
Comment #17
Wim Leers#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!
Sure; that's why I reach out to you by offering tests. Make the tests pass and I'll commit it :)
In that case, see my answer to #13.
#15: I already anticipated this remark; reply is already at #10:
#16:
Again, see my answer to #13.
Comment #18
gregglesI appreciate your perspective that the dev should have fixed this. It did not.
Comment #19
Wim Leers(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?
Comment #20
gregglesSadly I don't have time for that.
Comment #21
Wim LeersWell, 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?
Comment #22
gregglesIf 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.
Comment #23
Wim LeersI 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.
Comment #24
Wim LeersMy deity, am I a retard or what? I simply reuploaded the #10 patch, stupid! Here's the right one.
Comment #26
Wim LeersAs 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.
Comment #27
Wim LeersTo 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.
Comment #27.0
Wim LeersMissing the code tag