Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When trying to generate an image derivative from a missing source file, we currently return an HTTP 500 status code. From a system administrative perspective, 500 errors are bad, something is broken. However, in this case, what's broken is that the source file is missing. There's another HTTP status code for that, ... and it's a 404.
Comment | File | Size | Author |
---|---|---|---|
#57 | 927138-57-lighter404.patch | 1.72 KB | JKingsnorth |
#50 | 927138-50-lighter404.patch | 1.7 KB | Taz |
#46 | image404-927138-46.d8.patch | 1.71 KB | David_Rothstein |
#45 | image404_drupalexit-927138-45.patch | 1.61 KB | manarth |
#43 | image404_drupalexit-927138-43.d8.patch | 1.71 KB | manarth |
Comments
Comment #2
catchMakes sense to me, you can hit random style paths and generate 500s at the moment.
Comment #3
douggreen CreditAttribution: douggreen commentedrolling from drupal root this time
Comment #4
cyberswat CreditAttribution: cyberswat commentedComment #5
marcingy CreditAttribution: marcingy commentedReroll and move to d8 as it needs to be fixed there first.
Comment #6
rasmusluckow CreditAttribution: rasmusluckow commented#5: fail-image-generation-with-404-927138-5.patch queued for re-testing.
Comment #8
rasmusluckow CreditAttribution: rasmusluckow commentedI have rerolled the patch for D8 and simplified it a bit.
I have manually testet that I get a 404 if the source file does not exist, thus quitting the script early.
Comment #9
h4rrydog CreditAttribution: h4rrydog commentedComment #10
Albert Volkman CreditAttribution: Albert Volkman commentedThanks for the updated patch, @rasmusluckow! This patch indeed properly returns a 404 instead of a 500. However, one small nitpick. There's an extra space in your closing bracket. Please re-roll the patch with that fix and I believe this should be RTBC.
Comment #11
h4rrydog CreditAttribution: h4rrydog commentedAlso manually confirmed that the patch works, resulting in a 404 instead of a 500. This was tested on Ubuntu with nginx / php-fpm.
Comment #12
h4rrydog CreditAttribution: h4rrydog commentedCrosspost. Updating status.
Comment #13
rasmusluckow CreditAttribution: rasmusluckow commentedFixed indentation.
Comment #14
h4rrydog CreditAttribution: h4rrydog commentedCool. Indent fixed.
Comment #15
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thank you.
Comment #16
webchickHere's a re-roll I did on stage at Drupalcon Munich. I hope it works. :)
Comment #17
bas.hr CreditAttribution: bas.hr commentedIt won't work :) check here for D7 patch #1226512: Image module reports missing images as error 500 (server error) instead of 404 (not found). :)
Comment #18
webchickI'm an idiot. This is Symfony code. :)
Comment #19
Albert Volkman CreditAttribution: Albert Volkman commentedMarking this complete since imagecache isn't core in D6.
Comment #20
webchickIt is in D7 though. :)
Comment #21
Albert Volkman CreditAttribution: Albert Volkman commentedAh, my fault. I thought it had been resolved in the other issue.
Comment #22
droplet CreditAttribution: droplet commentedComment #23
h4rrydog CreditAttribution: h4rrydog commentedThis patch works for me in D7, properly returning a 404 (after patch) instead of a 500 (before patch).
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedWould it not be better to use drupal_not_found()?
Comment #25
webchickOops. Yep, I think Albert is right. Take a look at http://api.drupal.org/api/drupal/includes%21common.inc/function/calls/dr... for some examples.
Comment #26
handrus CreditAttribution: handrus commentedPatch using drupal_not_found().
Thanks for the suggestion and guidance.
Comment #27
handrus CreditAttribution: handrus commentedChanging status and re-submiting patch
Comment #28
handrus CreditAttribution: handrus commentedAs @caiovlp suggested I changed exit; for drupal_exit().
Thanks for reviewing this Caio!
Comment #29
caiovlp CreditAttribution: caiovlp commentedPatch #28 works fine and looks good.
Comment #30
manarth CreditAttribution: manarth commentedThe patch doesn't differentiate between a missing source-file, and an error preventing the image-style applying (such as corruption in the source image).
This is an important distinction, because if the image is missing, 404 is semantically correct, whilst if the image exists but cannot be converted because of a server error, 500 is more appropriate.
In #1226512: Image module reports missing images as error 500 (server error) instead of 404 (not found). we used an
is_file($image_uri)
check to serve 404 only when the source-image is missing - http://drupal.org/files/source_image_exists-1226512-19.patchComment #31
manarth CreditAttribution: manarth commentedNB: That
file_exists
behaviour is used in D8 (commit from #15 / diff)Comment #32
manarth CreditAttribution: manarth commentedThis patch checks if the source-image exists, and only returns 404 if the source image is missing.
I've also added a test for that.
Comment #33
manarth CreditAttribution: manarth commentedThis patch adds the same
watchdog()
message that's used in D8.Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedShouldn't this be
return MENU_NOT_FOUND
? (see http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_not_found/7)Does the test need to go in Drupal 8 too? (Also, minor issues with it: "source-image" => "source image" and "existant" => "existent")
Comment #35
manarth CreditAttribution: manarth commentedGood points, I've updated the patch to use
MENU_NOT_FOUND
, corrected source-image/existant, and rephrased the assertion message slightly.I've also created a patch to add the test to D8.
Comment #37
manarth CreditAttribution: manarth commentedComment #38
David_Rothstein CreditAttribution: David_Rothstein commentedThat looks pretty good to me, although is there a reason for the triple slash in the tests ("public:///foo.png")? Let's see if the tests pass for Drupal 8 anyway.
In the watchdog message, it looks like it's using the wrong variable (derivative file rather than the original image file), given the way the sentence is worded? Actually both would be useful in this log message, but the original image file is most useful since that's what's missing. Looks like the committed Drupal 8 patch had the same problem though...
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commented#35: image404_drupalexit-927138-35.d8.patch queued for re-testing.
Comment #40
manarth CreditAttribution: manarth commentedThe triple-slash? That's me spending too much time writing provisioning scripts where the triple-slash is required:
puppet:///modules/foo
:-DFor the watchdog message, if we're going to change it, we should change it for both D7 and D8, so it's consistent.
How about this?
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedAh, it's been a while since I've used Puppet, but that makes sense :)
The watchdog message looks good although instead of "using derivative path" I might say something more like "while trying to generate derivative image at"... I think it makes it more clear what's going on.
Comment #43
manarth CreditAttribution: manarth commentedHere's the D8 patch with the test, and the changed watchdog message.
Here's the message as it appears in watchdog now:
I've also corrected a mistake I made in the test, I originally had:
That second line shouldn't be there…instead we simply generate the URL with this:
Comment #44
manarth CreditAttribution: manarth commentedComment #45
manarth CreditAttribution: manarth commentedAnd the D7 patch.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedThe Drupal 8 patch in #43 looks ready to go to me.
(I'm just fixing an instance of "Test" => "Tests" in the function documentation in the attached.)
Comment #47
alexpottCommitted f816c89 and pushed to 8.x. Thanks!
Comment #48
handrus CreditAttribution: handrus commentedWhat about the backport to D7?
The patch look good to me.
Comment #49
aviindub CreditAttribution: aviindub commentedI just deployed the d7 patch in production. i can't vouch for the .test file, but the .module part of the patch is working as intended.
Comment #50
Taz CreditAttribution: Taz commentedRe-rolled the above patch to use a lighter 404, as per the rest of the image.module
Surely there's no need to cause a full 404 page render for failed images and put a hit on the server.
It's very easy to DDos a Drupal site under the image styles path without this patch, thus I am raising the priority of this.
Moving back to 'needs review' as this is a new patch for the testbot & community.
Comment #51
David_Rothstein CreditAttribution: David_Rothstein commentedHow is this a particular DDos risk? If we return a normal 404 I don't see how it's different from any other path on a Drupal site that returns a 404...
I can see the logic for wanting to return a fast 404 anyway (same reason as in drupal_fast_404()) but to do that we'd presumably want to respect some of the settings in the drupal_fast_404() function, plus it would need to go in Drupal 8 first. So perhaps we should get the original patch in first, and discuss that in a followup.
Comment #52
catchSome load balancer configurations will take a web server out of rotation if it returns a certain number of 500s. Returning a full 404 page isn't any worse than anything else agreed.
Comment #53
cdenneen CreditAttribution: cdenneen commentedI know it's been a few months since latest comment. Can anyone tell me if this patch is complete? Will it make it into 7.x?
Is it safe to use: https://drupal.org/files/927138-50-lighter404.patch
Comment #54
mgifford50: 927138-50-lighter404.patch queued for re-testing.
Comment #55
mikeytown2 CreditAttribution: mikeytown2 commentedDid a search and This is the only issue I found semi-related to this one that I just created #2267639: Image styles do not get created if the original file contains a plus sign or is double url encoded
Comment #56
JKingsnorth CreditAttribution: JKingsnorth commentedJust to say, this would be a great change but could we have a more useful 'on-screen' error message too, rather than just 'Error generating image.'
How about:
'Unable to generate the image because the source file does not exist.'
Note that this only applies to the currently still under review Drupal 7 patch. The Drupal 8 patch already does this.
[We've just been scratching our heads over this error for ages, checking server configuration and .htaccess files for the cause. But it was the simple problem that the images didn't exist on the server. A more useful error message would have saved us a lot of time (as would taking a proper copy of our /files/ directory)]
Comment #57
JKingsnorth CreditAttribution: JKingsnorth commentedThe attached patch makes this small amendment (better on-screen error message) to the patch in #50
Comment #58
mgifford@John, so you just added ", missing source file" to
print t('Error generating image, missing source file.');
Simple enough change. Still a big change from #45 though. Would probably be simpler to just have #45 RTBC and open up the variation in #57 as a new issue as it would also need to be dealt with in D8.
We still need a follow-up if we want to stop "full 404 page render for failed images". As @David_Rothstein suggests, if there is a real concern with DDos with the code in D8.
Comment #61
mgiffordComment #62
JKingsnorth CreditAttribution: JKingsnorth commentedFair enough, just thought I'd get it out there =]
Comment #63
mgiffordThat's totally fine.
Comment #64
FluxSauce CreditAttribution: FluxSauce commented#57 works for me on Drupal 7.32.
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedIt seems like there's a bit more consensus on #45 for now, so let's go with that one (we can deal with further refinements as a followup - especially since as I mentioned above this really seems like it should integrate with the drupal_fast_404() settings if it's going to do a fast 404). Will retest to make sure it still passes tests.
Although it is true that #57 is closer to what went in to Drupal 8. The on-screen "Error generating image, missing source file" message doesn't make lots of sense to me though; that is a very technical message to potentially show an end user. The technical message (with much more detail than that) is already in watchdog.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!