Drupal should not handle 404 for certain files

Here is another version.

It adds .ico to the list.

Links to mailing list discussion on this, all +1:

http://lists.drupal.org/pipermail/development/2006-August/018180.html
http://lists.drupal.org/pipermail/development/2006-August/018197.html

Quick question based on the mailing list discussion-

Assume I want to be able to set up path aliases ending in .html or .htm becuase I've replaced static site content with Drupal content. Will this patch prevent that from working? It seems that it will, so I'd suggest separating out these sorts of endings (shtml, html, htm) into a separate rewrite statement and making it commented out by default.

I opened another issue before getting to the end of the thread....

The original handbook page was written with reference to Drupal 4.6.x.

As discussed at http://drupal.org/node/56773, in 4.6, we made some edits to the default .htaccess file to allow the web server to handle bad file requests for most common file types.

Before this is standardized, some more testing is needed. Attached is out current .htaccess file.

@agentrickard - that attached file does not correspond to the current .htaccess. Did you add more to it?

Let me clarify:

That file is the .htaccess file that we use in production (Drupal 4.6.9), which inspired the original Handbook page. It has been modified since the original handbook page was written.

I added it for reference.

For what its worth, I am using the patch attached in #1 on my live sites, and there is no ill effect at all ...

It does not seem to catch them all though ...

I changed it to this, but don't know if it works or not yet.

@agentrickard: I don't see how your .htaccess (from #4) is really applicable to Drupal core since you simply disable Drupal's 404 handling altogether:

The original version of this patch (and that which is in the handbook) works for nonexistant files requested from directories that actually exist in the filesystem. It does something like this:

Request: http://site.com/misc/image.gif

1. Look for directory /misc/.
2. /misc/ is found, so look for image.gif inside of it.
3. image.gif is not found, and image.gif matches the FilesMatch regexp, so return Apache default 404.

Request: http://site.com/some/image.gif

1. Look for the directory /some/.
2. Directory not found, so the URL gets rewritten to http://site.com/index.php?q=some/image.gif
3. Bootstrap Drupal, look in menu, not found, return Drupal 404 page.

The only fix I'm able to think of for this would be to modify index.php to intercept files using the same regexp from the FilesMatch directive, and return 404 from index.php. I don't know if the powers that be will buy such a solution.

Oh, I think they will, given large sites having lots of 404 really suffer because of that false bootstrap, and too many of them stressing the system.

I don't have the time to devise a patch, but will be happy to test one on my sites, and perhaps a large client site too.

Here is what seems like an optimal solution. We keep the stuff in .htaccess as it is because having Apache handle things without invoking PHP is ideal. For the cases that Apache can't get to because of the rewrite rules, we run the same regular expression as the first line of index.php and return our own 404 if it matches. The results are as follows:

RequestResult
/misc/blog.pngApache will serve the image because it exists.
/misc/blogxyz.pngApache will serve a 404 based on the .htaccess rules because the directory exists but the file doesn't.
/foo/bar.gifDrupal's index.php will serve a 404 based on the rules there because the request ends with .gif. The very fact that we are in index.php means Apache has already decided it is a 404 case, with or without the modification to .htaccess (so the modification to index.php is not an Apache specific hack).

As long as developers don't start making paths in the menu that end in .gif, .png, .swf etc., we're fine. Does anyone know of any cases where such paths are generated? Perhaps by CAPTCHA or watermarking?

So much for my nice table.

/misc/blog.png
Apache will serve the image because it exists.

/misc/blogxyz.png
Apache will serve a 404 based on the .htaccess rules because the directory exists but the file doesn't.

/foo/bar.gif
Drupal's index.php will serve a 404 based on the rules there because the request ends with .gif. The very fact that we are in index.php means Apache has already decided it is a 404 case, with or without the modification to .htaccess (so the modification to index.php is not an Apache specific hack).

Here is an updated patch.

It picks up from what Robert did, and changes the following:

- Adds .ico file as a type to be ignored.
- Makes .html as a commented out line with instructions. The reason is, some sites use the .html in URL aliases for various purposes (e.g. content migrated from older systems, making awstats give better stats by type, ...etc.)

I tested this and it works as expected. Should cut down resource usage on some busy sites with lots of flat files.

So, +1 from me.

What if I'm serving images from the database, using the 'private file method'?

What if, in the near future, we want to do access control on images associated with nodes? (Something we should do.)

This breaks private files. Ugh.

Of course, we could update the regexp to accommodate the private files path... but what about filemanager files? And ecommerce files?

Yuck.

I don't know how we're going to introduce the access permissions on files attached to nodes (filemanager/attachment already has a solution for this), and don't think that it should be part of this patch.

Is the best thing to do to make the change in .htaccess and leave it at that? I suspect that we're only covering 5-10% of the target cases if that's the route we go. The other option would be to build the check into the bootstrap process and hope that we could do a partial bootstrap for 404 images instead of a full bootstrap, but this is clearly inferior.

I'm writing something maybe related, http://drupal.org/node/77514. It performs a search for the missing URL with certain words stripped (eg html, php etc).

I wouldn't be surprised if we end up introducing a second entry point to the bootstrap process, something like files.php, that handles any URL that has a file ending. That file would initiate an alternate bootstrap process where one of the first checks would be "Is this really a file that exists?", and right after that would check "Is this a path that is handled by a module?".

Haven't Walkah and Co. been doing some thinking on the file bootstrap issue? What is their concensus?

i posted some thoughts on private files at http://www.tejasa.com/node/113. no code though.

IMO, the best we can do is to try and serve a lightweight page. Dropping the sidebars on 404 pages could save a fair amount of resources; both bandwidth as CPU cycles.

i guess that will have to do for now ... i've been meaning to add a block-less page anyway. needed for panels.module ... if anyone has ideas on how to implement this, please chime in. assigning to self.

Introduce a new theme function? mini_page based on theme_page() in theme.inc?

theme_maintenance does something similar. That or provide a drupal_is_404() function for the template files? Food for thought. :)

Thats was easy. i just added minor conditionals in phptemplate.engine and chameleon.theme. Is used by 404 page, and will certainly be used by panels.module - http://angrydonuts.com/the_dashboard_revisited

To omit blocks, just pass FALSE as 3rd argument - for example theme('page', $output, FALSE)

oops - here is the patch

It begs the question: do we need to redo the theme_maintenace() page using this system? Can, and if so, should we merge them?

Here's the .htaccess portion of the patch. We should adopt this part regardless of the solution that we come to for the rest because it is the most effecient way to handle requests for files that don't exist.

i looked into maintainence_page and install_page but they are specialized and merit own function. They avoid calls to functions that haven't been loaded yet. I think we can leave them alone.

I'm ok with Moshe's patch. My original thought was to introduce a helper function (drupal_is_404()) but I guess this works too. A drupal_is_404() might be a little more powerful, and more transparent though.

Then again, the extra paramater passed to theme_page() might, for example, be useful to simply certain pages (like the welcome page after installation).

i'll take some more feedback from cvs commit team, or proceed with commit. the extra param on theme_page() is vital for panels.module (whichis super cool if you haven't looked yet - http://drupal.org/files/issues/htaccess-404.patch_0.txt)

Which patch are we talking about? I can't tell from moshe's review if he thought his own patch was ready for commit or Robert's.

i am talking about my patch. i have not tested robert's patch.

Updated patch... includes Moshe's patch + Roberts + update for HEAD.

Tested and working well. I really like the fact that blocks go off when you get a 404, that really can save a lot of bandwith + CPU cycles.

Also, the Apache 404 for actual files is *very* necessary. If you have a link to http://www.example.com/somethingFunnyLooking.gif ... you are linking to a file directly--you are expecting either the file or a 404, *not* a full Drupal boostrap to give you a 404. 99% of the time that link is going to be in the theme or some link posted by someone and they'll never see that 404... so let's give them the absolute least amount of overhead with that 404.

I'd say this is ready to go, nothing new here and we solve 2 problems at once: 404s for content in Drupal (no blocks, less CPU, SQL, etc..) and 404s for files (*nothing* at all).

Also when/if this goes in, we need to update the theming docs about this new $show_blocks variable so themers can be sure to use it so save some CPU cycles on 404s.

Talked with Moshe in IRC, we both agreed this is RTBC. It was ready before Neil just didn't know what to commit (now it's both).

This is two separate issues. Make a separate issue for one half of this.

Neil, ok here is the first patch then, this is for the blocks. Note, this will need a documentation update.

This patch is the same from before, just synced with HEAD.

After the patch in #36 is commited (same as in #24 just updated for HEAD), here is the next patch to address the .htaccess and 404.

Please post different patches on /separate/ issues.

I moved the most recent version of Moshe's patch to a different issue since the title of this issue fits my patch the best:
http://drupal.org/node/80201

Here's the 404 patch which is for this issue :-)

Thoroughly tested, this is ready to go. Like I said in #32, this is needed for files, it can save a ton of wasted Drupal bootstraps and server overheads.

Because of this (two) patches I think the need for http://drupal.org/node/76824 has increased, since the 404 page just "blanks" ATM. Most users wont configure a 404 page...

The nearly-identical commented lines next to the uncommented need a better explanation.

From what I can tell this still breaks private file downloads.

sad to see this one languishing. if it cam edown to it, i would add this and put some doc for private files tha tthis must be commented out.

moving from x.y.z to 6.x-dev version queue. Hopefully this will also bump the feature back into people's attention.

color.module also does not work for private files and it is a quite prominent part of our default theme. see http://drupal.org/node/92059

i think private file incompatibility is acceptable for drupal 5. we will fix that in 6. anyone want to address drumm's last comment and resubmit?

-1 on this addition to the .htaccess, It will break any dynamic generation of the restricted file types & drupal based file access control.

If it is added I'd like to see it commented out by default, so people who have concerns about bandwidth and server load can uncomment them. We could also choose a special case for files/ or system/files.

Here's and updated patch which cleans up some stuff.

Known issues:

• Regexes need to be updated to ignore files that live in files/ and system/files/ --- this will fix both private downloads and modules like imagecache which make special use of the files/
• Directories that do not exist (e.g., themes/abc/something.gif) cause an Apache 404 error but then it says it can't find the proper 404 document. However, if use a directory that exists /themes/something.gif with an image that doesn't exist, it gets the proper 404

The problem with 'files' is that it can change and we don't have access to file_directory_path from the .htaccess file...

it is reasonable to force folks to have 'files' in the difrectory path, like we do for modules and themes. but if not, i htink this can go in commented out and ask folks to enter their files path.

Here's an updated patch. Moved definition of ErrorDocument below our existing definition (otherwise it'll just get stomped on by the later ErrorDocument 404 index.php line).

Changed the description a bit. Commented the lines out per #50, except the RewriteCond.

Don't have time to test this just now, but will not handling rewriting for .html files mess when you have node/2 aliased to about.html, for example?

Also, in those new comments it might help to have a reason why one might NOT want to uncomment those lines. For example, why does this come commented-out by default.

Here's an updated patch with a regex from Steven on IRC to ignore those files when they exist in /files which can interfere with private downloads and imagecache module.

Regex needs some tweaking though, almost there.

Also uncommented the regex because if it can ignore anything in files/ it shouldn't break anything. Feel free to comment out though if I missed a use case.

m3vrck: FilesMatch does absolutely nothing with Directories. Scanning against files/ won't work. The only way forward on that avenue is to get the default handler into the .htaccess that is generated into the files/ directory.

And files isn't always named 'files'.

How about a new approach to this issue that has been going back and forth for so long, rolled back, ...etc?

Instead of doing things in .htaccess, requiring people to uncomment things, ..etc., can we move this into Drupal itself?

Can we do a light bootstrap (as early as possible), read a variable (perhaps in settings.php) that has a pattern, try to match it, then exit early?

This is of course more overhead than letting Apache handle the whole thing, but it would still be better that what we have now, provided we do it as early as possible.

Comments?

@kbahey - now is not the time to switch gears on this issue. thats a radical change and quite debateable.

@drumm - i think a code comment in htaccess and possibly where we set the files directory is enough to satisfy the rare case where files dir does not end in 'files'.

moshe, the reason I suggested an alternative is that this is dragging on, due to various issues that are pointed out by various people. So, getting consensus on it will be difficult.

So, in my view, it may not make it to 5.0.

My suggestion was to move it inside drupal, so we can manage the patterns to exclude, ...etc. from inside Drupal with a little bit of sacrifice on the overhead side.

If others think it will get in core via htaccess soon, then I am happy to go with that too.

anyone care to carry this forward?

Anyone willing to work on this performance enhancing patch?

The code freeze is a few weeks away ...

Can someone answer my question at http://drupal.org/node/76824#comment-173567, which is the same as #3 submitted by pwolanin? What if I have an alias for .html pointing to a missing node? Not that that is a huge deal, just would like clarification. Also, the hardcoding /files issue is still there. What about for multisite?

@kbahey, I agree that moving this inside Drupal would save us a lot as we could get the right files dir and some of the other issues might be alleviated. But, maybe that would soften the performance gain that this is trying to achieve. Since this is a bit funky, I think the easiest way is to put this in .htaccess and comment it out initially, allowing users to turn on this boost if they want. What do you think?

Many of my site has .html as the suffix, mainly so awstats can give good stats on them.

I think that this patch is for performance more than anything else. If we avoid a Drupal bootstrap, we waste less resources when we have a 404.

So, the options are:

1. We add the stuff in .htaccess and index.php and comment it out. This is the path of least resistance and would give us what we need for D6.

2. Have .htaccess generated automatically from Drupal like we do with the CSS cache stuff. A default setting provides which file extensions to use and the admin can override them (e.g. remove html or add xyz)

The .htaccess need not be writable. We can take a non-fancy approach and just display the .htaccess contents on the screen and ask the admin to copy/paste, or we can write to the tmp directory and ask the admin to move the file manually. Since this is an advanced feature, I think this is acceptable.

3. Move parts of that in a lighweight bootstrap, as I suggested earlier.

I think 1 will have the best performance. If Dries is amenable to this, I can work on this towards end of next week.

I agree that #1 is best for now. I hope kbahey or other have a chance to revive this.

I hate to see Drupal 6 go out the door without this simple yet effective feature.

Here is a patch for current HEAD.

Here is a patch that moves the index.php stuff to settings.php, and comment it out.

This way, we have no cruft in index.php, and those who need this can just uncomment it (albeit in 3 different places, but better than a whole Drupal bootstrap for a measly 404).

works as advertised. i think this is a good compromise, and all the lines are needed.

One of the chunks is double commented, the other is not.

Here it is, fresh from the microwave ... ;-)

I think this patch is important (I wrote the original handbook page).

Circling back, have you tested against http://drupal.org/node/56773#comment-243479? And against file uploads in general?

Reread the htaccess comments, there are typos and repeated stuff as mentioned by Steven. Also if the functionality in htaccess is effective, there would not be a way to get to the default settings.php change. Are three three marketed as an 'all or nothing' solution to uncomment all three to cover all bases?

The 3 are needed but for different reasons. When a directory exists but a file in it does not, then Apache is able to handle the 404, and hence the .htaccess changes.

However, if the directory does not exist, then Drupal is still bootstrapped, so we exit as early as we can, and hence the settings.php change.

Rerolled the patch. Hopefully comments are OK now. I also changed it to use REQUEST_URI instead of QUERY_STRING, since that is what we are checking in .htaccess.

Also added a small "Page not found" message.

* What's a "flat file"? I know what a flat file is. It's not what the documentation prefaces.
* None of the sentences that end with "settings.php" have a period at the end.
* None of these lines are commented (and they should be).
* "corresponding the RewriteCond" has an extra "the".
* I can agree that we'll save "server" resources, but not "bandwidth" resources.
* "corresponding FilesMatch line" - it's actually three line[s], not just one "line".
* Technically, if a number is less than 10, it should be written out ("three lines", not "3 lines").

I'm not sure I like this third check. I worry about dynamically generated images (ala CAPTCHA or various statistics) - I suspect, just off the top of my head, that one of the "make an image from text" modules will fail, like textimage.module. This third check also defeats a request for an image uploaded to a node when private downloads are enabled (URL path is ?q=system/files/1608.png, which triggers your check).

And note that #74 and the "private downloads" issue was brought up as far back as #14.

I have trouble believing that the performance gain is worth completely eliminating file extensions in the virtual file system. The whole simple beauty of the "no file named $1? let index.php?q=$1 handle it" method is that there is no difference from the outside between the Drupal handler and the files served directly.

What performance gain is this supposed to give, anyway - it seems to speed up only 404 errors. For a site without broken links and sensible, persistent URLs, shouldn't those be the exception rather than the norm?

Just my two cents there...

What performance gain is this supposed to give, anyway - it seems to speed up only 404 errors. For a site without broken links and sensible, persistent URLs, shouldn't those be the exception rather than the norm? - Not really. Given enough time, your site will be constantly hit by broken spiders, endless requests for favicon.ico (which may not exist), and various cracking attempts via known vulnerabilities in IIS and other webservers. My top WEEKLY 404s are 14045 /favicon.ico(has never existed and never will), 2296 /popular (never existed. broken bot), 1668 /ghostsites/images/pixel.gif (probably once existed.), 1414 /popular/alltime (never existed. broken bot), 1025 /images/pixel.gif (probably once existed), 467 /cgi-bin/isp-post/isp-post.cgi (last existed 7 years ago. bot attempting exploits), 409 /files/users/picture-11.jpg (existed once. user, and all his content, was deleted.), 178 /db/zone.htm (never existed. no idea where this comes from).

@Arancaytar

Just for background: This is a massive problem for sites that migrate to Drupal from a flat-file system. In the case of SavannahNOW, we had thousands of now dead links in Google, Yahoo, et. al. We were basically getting DOS'ed by 404 errors.

Plus, if someone requests a page that no longer exists, and that .html file contains two .css and 10 .gif files, that turns into 13 404 errors that "fall through" to Drupal. In effect, Drupal spends all of its time searching for files instaed of serving pages.

It is also a generic problem with bots and spiders -- particularly spambots and hackers. Our traffic logs show repeated requests for .dll and .cgi files that point to known or suspected exploits in IIS.

All that aside, the case may be that the best path here is a well-tested and documented handbook page, with a reference in the core INSTALL.txt that points to those instructions. I say that simply because edge-cases seem to be drowning this issue. If we can't fix all the edge cases, I doubt this will get into core.

Here is the wording, before I roll a patch again:

.htaccess part1:

.htaccess part2:

settings.php:

Regarding private download, we can add a comment that this does not work in such a case. How about that? Any wording suggestions Morbus?

CSS is a acronym and should be capitalized. I'd much rather these sentences be merged into one paragraph (for each block).

As for the settings.php, I can't support this patch with that block in - adding wording-to-the-wise about private downloads doesn't solve the issue of textimage (which creates dynamic images) or any other module that attempts to do something dynamic with normally static files. It's something that would cause more confusion and endless "your module is broken!" requests.

I support the .htaccess additions.

Morbus is right about private file download. This is something we need to think about. However, that should not stop the .htaccess parts.

Not sure what merging sentences in one block for each paragraph means.

I am unassigning myself from this issue. Someone else can take it and run with it.

In order not to break textimage and imagecache, I just added this condition in the settings.php if block:

i know that one could use a files dir that doesn't begin with 'files' but thats a bug in core IMO. we require modules dir to be modules, themes to be themes, and we have every right to require that files be files. i just need to submit a patch for that. if folks agree, i'll get started.

note that color.module does not work with private files and thats a far more important feature than these chunks of commented out code that are proposed.

Given that any module (or even custom site code) might attempt to "fake" a file extension in the URL, it seems like this fixes one issue (404 overload) at the cost of introducing an uncounted number of problems whose cause cannot not readily identified.

"Why is such-and-such a page suddenly returning 404 errors?" seems like a far more difficult support issue to solve than "Why is my Drupal site constantly running into my host's server load limits?". When the server overloads, you have a very obvious problem that will probably be handled by an admin who can poke around in .htaccess files. The other one is encountered by users who have no idea what happened.

Because of this, I'd propose that the relative addition/change in .htaccess be commented out with a description of how to activate it, and document this as a performance tweak in the handbook...

subscribe

Where has this issue been all my life? Subscribing.

Subscribing

Subscribing

Moving feature requests to D7 queue.

For handle file in non-existent directory I use rewrite to /404.ext in .htaccess. Put

@zeezooz, can you roll a patch?

New features are going into 7.x-dev, not 5.7.

The trickiest thing to work around here are modules like imagecache that rely on the 404 handling to auto-generate versions of files that don't yet exist. I wonder if we could get around this by having them add a querystring paramete (e.g. 404handler=drupal) that will cause .htaccess to use the Drupal 404 for these files, rather than the lightweight static 404 page. This way we could potentially have the rule enabled by default, and still allow these modules to work without having people hack the (fragile!) .htaccess regexi.

Anyone fancy taking a shot at this?

patch for #89

Note: Favicon.ico just got such special treatment in Drupal 6.3.

patch #92 works great!

We are using this on MothersClick with imagecache & imagefield with *no* problems at all, test it yourself:
http://www.mothersclick.com/no-directory/this-really-works.css

The patch that went into D7 and D6 for favico should be reverted in favor of this patch which works great.

Still applies with fuzz, back to needs review.

I agree 100% that a more generic solution would be great for this issue rather than special casing favicon.ico - it's a big performance issue on some sites (including one I run) that have old legacy linkrot for images etc.

Could we leave default behavior as it is? This will break functionality on a great number of sites - whether you are using a private file system, or any module that dynamically generates file downloads, or even if you are in the habit of adding ".html" to the URL aliases of your nodes.

It's a fine tweak, but I don't see that we should knowingly and without warning or explanation break the sites of one part of the user base in order to improve performance for another. These lines should be commented out initially, so you can uncomment them if you need the performance. We don't default to aggressive caching either, for the same reasons.

This patch DOES NOT break dynamically generated files like images with imagecache. Like I said, we're using it on MothersClick and we rely on imagecache and everything works a-ok. Before saying a patch doesn't work, test it. There is a line in there that says ^/files/ ...

I'm sorry, I often get mod_rewrite rules wrong. Does the following prevent the file being handled by Drupal, or is it an exception for files that are handled by Drupal?

Related: #295456: Bug in .htaccess file.
Also the patch Gábor mentions in #93: #174940: Root /favicon.ico accounts for majority of 'Page not found' errors.

Is it proposed that *by default* Drupal should not handle 404s for the image file types? Two comments:
- if Drupal doesn't handle them then I lose an easy way of finding broken img tags etc. on my sites
- the comment sections e.g. # The following lines prevents [sic] ... # You must uncomment the corresponding RewriteCond and RewriteRule lines later in this file. suggests that by default these lines will be commented out??

@98, and for my own edification: What I think those 2 lines do is the following:
- first of all, %1 is a backreference to the file extension of the image file (as matched the line before in the RewriteCond)
- so RewriteCond %{REQUEST_URI} !^404.%1$ makes sure that the request was not for 404.jpg say (this is needed so that the next line doesn't create a loop)
- RewriteRule ^(.*)$ 404.%1 [L] then rewrites the current request (e.g. picture.jpg) to 404.jpg and ends ReWrite processing (so that the following rule - Drupal's normal one - doesn't get applied).
- only then does the FilesMatch directive get applied

Actually I'm not sure the request URI needs to be rewritten ... just need to make sure that Drupal's normal RewriteRule doesn't get run?

The last submitted patch failed testing.

#100 was the testbot problem #335122: Test clean HEAD after every commit.

Here is a rerolled patch that does the following:
- Reverts the favicon.ico rules, because they now are handled by these rules
- Adds some additional extensions for common 404s (mostly exploit-bots)
- Changes the 'files' rule so it does not need to be at the start of a line. This is needed so we can catch sites/default/files paths (the default since Drupal 6). This also fixes (AFAICS) the case where private files are enabled.
- Adds a help text and a validation rule to the file system settings to ensure the file system path contains the word 'files' somewhere (as suggested by moshe in #82).
- Removes handling for htm/html files by default, because migrating from a static html site is a pretty common use case. I have added comments and sample lines for people who know they don't/won't use these aliases, but by default you don't have to do this (and you still save a large number of CPU cycles).
- Makes this enabled by default. I don't think it's worth including this if it is going to be disabled by default, especially if it means uncommenting 2 far apart sections of code in a hard for newbies-to-grok file. If we are not going to enable it by default this could live just as happily in a documentation section for high traffic domains.

Some thoughts:
- This opens up the possibility of a contrib module to save static versions of the error pages as 404.whatever (a bit like boost module does for regular pages), or introduce some lightweight way of logging these errors (404-handler.php) with a minimal change to the rewrite rules.

this could live just as happily in a documentation section for high traffic domains.

+1, though leaving it in as a comment will make it far easier to enable - just tell them to remove the comment on the documentation page.

Again: This change will speed things up for some people and break the sites of everyone using private file downloads, since you're not even using files/ or system/ exceptions, as well as anyone generating dynamic images with extensions, which can't be fixed with any additional special exception. Is it too much to ask not to break people's site by default?

@Arancaytar - it has been stated multiple times in the thread that this patch does not break imagecache or any similar modules and has been used successfully on large production sites that also use imagecache. If you read my post you would also note that this should now work with private file downloads. If you would like to find something that it does break, then please by all means try the patch and report back, but repeating "this breaks imagecache" over and over is neither accurate nor helpful :)

The last submitted patch failed testing.

It occurred to me that this would be a great candidate for some tests (which would help settle the what this does/does not break debates!) - any takers?

The issue I have with this patch is treats 404s for files differently than 404s for something in Drupal's menu system.

IMO these should be treated the same. The attached patch is how we handle on MothersClick.com we a single configurable file that both Apache and Drupal can use.

While not core worthy, hopefully this sparks proper direction for this patch.

What a cursed issue we have here.

Patch in #102 should not throw a form validation error. Instead, just use a note for the form element description, so site admins who need to use a path without "files" do not have to hack core.

Patch #107 looks interesting. Just an idea: Could we use a separate 404.php in the root directory, which performs a minimal bootstrap, checks the configured file directory path for the site in question, and short-circuits to a (optionally configurable) 404 output - or - if Drupal is supposed to handle the request, performs a full bootstrap?

I hate that D7 would be released still handling all 404s via an expensive bootstrap. This is a big performance suck.

The attached patch solves this in two ways:

- Adds rules to .htaccess to bypass 404s for certain files.
- Adds a safeguard in settings.php to stop very early if we are handling a static file.

Please please let us have this in D7. We can refine later if needed, but let us ship with something that prevents Drupal from handling static files.

The last submitted patch failed testing.

You want to write a test module which defines a callback in files, say 'test.gif' and check the return of it -- stuff a random string in a variable in the test and print variable_get in the test module.

Here is an updated patch, with 3 new assertions to ensure that the web server is passing "fast 404" responses to specific file type requests outside the files directory, and that Drupal can still handle requests within the files directory (system_test module now registers 2 imagecache style handlers for this purpose) for both public and private file serving.

I have also tried to make the comment help as clear as possible, although it is hard to balance clarity against excess verbosity - feedback is welcome.

I also took the settings.php fallback out - I am happy to add this back in if someone can explain why we should. This would only be reached on non-Apache webservers. We generally don't duplicate .htaccess functionality in php (especially for "optional" functionality such as this) - for example the www/no-www redirection could easily be done in settings.php, but is not. I think it is reasonable to request that people set up equivalent functionality in their non-Apache webserver configurations, and/or add code to their settings.php to do the same - we have places in the manual to document these.

The last submitted patch failed testing.

Hmm, I can't reproduce the test fail locally - anyone else care to test?

Owen, I do not get this, the rewrite rule changes the request to 404.gif or similar? Or what is the 404.%1 wanted to be? And, why wont the filesmatch grab your request, even in the files dir before any rewrite?

The rewrite rule simply prevents the request from being rewritten and passed to Drupal (if the file does not exist, the file is outside */files/* and has a specified extension). This causes Apache to 404 the request and look up the appropriate ErrorDocument for the request, which has been previously set for files with this extension. The filesmatch does not do anything at all unless Apache looks up an ErrorDocument for the request.

There perhaps could be problems with this if clean URLs are disabled - kbahey, did you check this at all?

I am wondering if this is a good idea. While it might be good for performance, you will never notice that some files are missing if you do nto additionally look into the apache log.

Yeah, honestly, I'm not hugely fond of this functionality - it's pretty hackish, and severly cripple's Drupal's own log functionality (which, over the many years, people have probably come to expect to be a literal equivalent to server logfiles). I don't doubt that heavy 404s can cause performance issues - I just don't feel this is the answer.

I'm wondering if we should just do either extension matching or file directory matching in drupal_not_found() - so log the 404 for site admins, but then return straight away if it's a file rather than a page. That'd save all the page building and rendering for the 404, but still give us logs.

It simply is not Drupal's job to catch all possible requests under its tree and log them. I like this patch a lot. catch's idea still requires a bootstrap which is a big problem. some contrib module can implement an apache log viewer if people need to view these 404's inside of drupal. or make a contrib module which lets you add a simple link to awstats/webalizer the reports block. web hosts could even make their own distro with that link pre-configured.

It simply is not Drupal's job to catch all possible requests under its tree and log them.

It has done so for years and considering everything this behaviour has caused very little problems.

I like this patch a lot. catch's idea still requires a bootstrap which is a big problem. some contrib module can implement an apache log viewer if people need to view these 404's inside of drupal. or make a contrib module which lets you add a simple link to awstats/webalizer the reports block. web hosts could even make their own distro with that link pre-configured.

I think it would be bad user experience to change this behaviour.

Catch's idea sounds interesting, but I am not sure how it could work.

Summary: I'd won't fix this if I were a core committer.

I don't like, at all, how the patch redirects the user to 404.$fileextension. This is *really* damaging - that's the actual entry that will get logged into the server log file, meaning that we are *crippling* a traditional error_log by filling it with garbage. How can I fix my broken links or embeds (etc.)., if Drupal is filling my log with "404.jpg" and "404.html"? This'll corrupt not just the error_log, but also anything that works on error_logs, such as Webalizer and so forth.

To amend my previous comment, here's what the error log looks like with this patch:

The first entry is the rewritten file - I had requested ahem.jpg. The second entry is a modern browser's standard attempt to get the favicon.ico (which, naturally, we can't really tell here, since it's been rewritten).

Huge -1.

Also has anyone tested this with imagecache?

catch: assuming that the Drupal 'files' directory has been set to 'files', then imagecache would be fine. If the 'files' directory was named something not 'files', then imagecache, private downloads, and other dynamic generations would fail. They could be made accessible again only if the .htaccess and Drupal source code was modified (fun, right?).

Should've read the patch again, thanks though Morbus.

@Morbus - could you recommend an approach then? Right now, a site can nearly crumble if a few bad invalid images get posted to the home page (for example)

Just a note I think we could do my approach via #444756: Add a page_not_found() hook to Drupal with a few modifications.

@moshe: I don't have a suggestion you'd like, no - I don't think it can be cleanly fixed at the .htaccess level (i.e., meaning we'd need to do /some/ sort of bootstrap; ala @catch's point). However, I also disagree entirely with your assertion that "a site can nearly crumble with a few invalid images" - that's ridiculous: as someone who has been running the same site for more than 10 years, and gets dozens of 404s every minute, (historical, rootkitish, and accidental, etc.) it's a bit FUDdish. To think that Drupal could get to where it is now, with such a easy 404 DoS, is inane.

I will agree, however, that certain shared hosts can have such a problem with a popular site and sliced resources. I just don't think, however, it's so incredibly widespread that Drupal sites everywhere are being crippled because of it. Do I think something should be done about it? Sure, I have no problem with that. Do I think it's critical? Nope. Do I think it's critical enough to a) strip an existing expectation from Drupal (404s in the reports) and b) break the equivalent functionality at the webserver/log level? Most definitely not.

Also, putting the notice about the missing images into the watchdog will allow site maintainers to create the missign images or to remove the broken node/template/whatever.

This issue is pure sillyness.

@catch - in this issue, we want to handle 404 without a bootstrap. really we want to handle a 404 without any php at all but definately no bootstrap. a 404 hook completely the wrong direction IMO. a 404 hook for the files directory might be fine and can be pursued in that issue.

Also we haven't been doing a full bootstrap for favicon.ico 404s since 28th June 2008, so this patch only fixes image files which aren't favicon.ico and aren't in sites/*/files. I think with something in drupal_not_found() we could skip rendering the page for files in sites/*/files as well.

>skip rendering the page
If page caching is on then the response will be from the page cache anyway, so there will only be a full bootstrap/rendering of the page if the request hasn't already been cached.

cool, so I need to check if favicon.ico exists myself? What a truly great idea. I'd expect something like this from WP or Joomla, but not from Drupal...

Comment 127 is not correct - the file directory path does not need to be set to "files", but simply needs to include the string "files" somewhere within it. There is a validation on the setting in the patch.

I agree that the current patch breaks Apache logging (404.ext) in an unacceptable way and is not committable. Possibly there is a way to fix this - I tried briefly to no avail. I think I am also leaning towards a php based solution, ideally with optional Drupal logging (and hence DB bootstrap). Either way, I DO think we need an improvement here - I have seen several site outages caused by 404s when assets on a single browser page view creates many Drupal hits, or the page has repeating AJAX callbacks that are 404ing.

Either way, I think 2 of the 3 tests I created are still worthwhile, since they ensure the imagecache style facility is functional (which I think could pick up some subtle breakages in the file path/url functions).

Owen - the RewriteCond specifically looks for "/files/" (unbounded), but the validation code only looks for "files". This would allow me to use "drupal_files" inside Drupal, and pass the validation, but lose the "fast" 404s.

I actually like to translate the default files path to use "dateien"...

If your system is prone to meltdown because of a few missing files I tend to think that this is not a Drupal problem per se.

You should evaluate your hardware and possibly buy more.

You also should test your site on a staging system to catch such 404s in advance.

@Morbus - correct, this was a bug - however the symptoms would be broken imagecache, rather than lost fast 404s.

Attached patch fixes this, and also fixes the issue with the tests failing when mod_rewrite is not enabled. It doesn't fix the Apache 404.ext logging issue however.

Sigh

Well, 5 broken images on your home page can be a nearly a 5x multiplier on your traffic. It leads to 5x on the number of bootstraps that your web and DB servers have to service.

Excellent reason to just fix the stupid images. Thanks to Drupal (as it is now) you'll at least be alerted to the problem.

You won't be alerted to the problem if the site is down due to 5x traffic

You can't solve the issue if your error log shows 5 entries for 404.jpg.

(See, we can do snarky replies too!) ;)

Snarky replies aside, I think everyone agrees that logging 404.ext for missing files is not acceptable.

What we need is either an alternate .htaccess approach (that logs 404s correctly, and works acceptably both with mod_rewrite enabled and disabled) - OR a PHP based approach that is sufficiently faster than a cached bootstrap to make it worth doing.

FWIW, I do this on my sites:

Than I usually set the path to sites/whatever/theme/whatever/404.php and include my logic there.

This is in addition to setting the htaccess to point to this same 404 file as well (and having similar htaccess work).