The default web.config file that comes with d7 includes this rule that doesn't check if the file actually exists or not.




So, if you have a perfectly good favicon.ico in your root directory, you get a 404.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

I have not do real test but it seems right. any suggest for fix it ?

daycrom’s picture

Temporally fix this disabling rule from web.config

From

<rule name="Force simple error message for requests for non-existent favicon.ico" stopProcessing="true">
          <match url="favicon\.ico" />
          <action type="CustomResponse" statusCode="404" subStatusCode="1" statusReason="File Not Found" statusDescription="The requested file favicon.ico was not found" />
</rule>

To

<rule name="Force simple error message for requests for non-existent favicon.ico" enabled="false" stopProcessing="true">
          <match url="favicon\.ico" />
          <action type="CustomResponse" statusCode="404" subStatusCode="1" statusReason="File Not Found" statusDescription="The requested file favicon.ico was not found" />
</rule>
Monochrome’s picture

Status: Active » Needs review
FileSize
671 bytes

Attached a patch to fix this issue. Adds a conditions to the rule that checks if favicon.ico doesn't actually exist.

Status: Needs review » Needs work

The last submitted patch, checknofavicon-1041580-3.patch, failed testing.

Monochrome’s picture

Version: 7.0 » 8.x-dev
Status: Needs work » Needs review
Monochrome’s picture

#3: checknofavicon-1041580-3.patch queued for re-testing.

Ali Mirjamali’s picture

Issue tags: +IIS, +favicon

Tested the above patch with IIS 7 and Drupal 7.15. I confirm that I works. Thanks for the solution.

Ali Mirjamali’s picture

#3: checknofavicon-1041580-3.patch queued for re-testing.

RumpledElf’s picture

Status: Needs review » Reviewed & tested by the community

Can someone just commit this before d8 comes out? It is possibly the most trivial bug in the entire of d7 and has been patched for aaaaaaaages >.<

dcam’s picture

#3: checknofavicon-1041580-3.patch queued for re-testing.

dcam’s picture

I tested #3 on the 8.x branch. Before applying, attempting to access the favicon resulted in a 404 error. Once patched, the favicon could be accessed normally. So this issue is still RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Committed and pushed to 8.x. Thanks!

This didn't apply cleanly, so markting to be ported for D7.

Suggested commit message:

git commit -m "Issue #1041580 by Monochrome, daycrom: Fixed Favicon.ico 404s on IIS."

(daycrom doesn't get picked up by Dreditor, but he made the suggested fix in #2.)

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
726 bytes

Backported #3 to D7.

Status: Needs review » Needs work

The last submitted patch, checknofavicon-1041580-13.patch, failed testing.

dcam’s picture

Version: 8.x-dev » 7.x-dev

Setting correct version.

dcam’s picture

Status: Needs work » Needs review

#13: checknofavicon-1041580-13.patch queued for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

I know that we don't RTBC our own patches, but I'm hoping this one might be an exception since I don't think there are many other core devs who have an IIS environment set up. If @David_Rothstein (who seems to be committing 7.x patches this week, which is why I'm taking this action) or anyone else feels I'm wrong for doing so, then set it back to Needs Review. If there is someone other than myself who has an IIS dev environment, please take a minute to apply it. It is pretty trivial to test.

This is the same basic change that was applied to 8.x. I can confirm that before applying the patch misc/favicon.ico could not be accessed. After applying the patch the favicon could be accessed normally, appeared on browser tabs, and appeared in browser bookmarks.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ebe236b

It looked safe to commit given the above testing and the fact that it's very similar to code that's already in that file. Although I'm curious why <conditions> was placed after <action> in this case, rather than before it like the others. If there's any chance that could cause functional issues in some particular setup, someone please let me know :) But I assume it's fine, and just a stylistic thing.

News4u’s picture

Status: Fixed » Closed (fixed)

this issue has been fixed in the new release 7.2.3
Note: JuST UPGRADE to th enewest version of drupal then run scripts to update the database
problem solved

David_Rothstein’s picture

Status: Closed (fixed) » Fixed

Thanks for the additional testing! This is a recently-committed patch, so please leave this at just "fixed" so it stays visible in the issue queue for people who might need to find it. (It will be automatically closed after a while once there are no more comments.)

Status: Fixed » Closed (fixed)
Issue tags: -IIS, -favicon, -Needs backport to D7, -7.23 release notes

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