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.
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.
Comment | File | Size | Author |
---|---|---|---|
#13 | checknofavicon-1041580-13.patch | 726 bytes | dcam |
#3 | checknofavicon-1041580-3.patch | 671 bytes | Monochrome |
Comments
Comment #1
droplet CreditAttribution: droplet commentedI have not do real test but it seems right. any suggest for fix it ?
Comment #2
daycrom CreditAttribution: daycrom commentedTemporally fix this disabling rule from web.config
From
To
Comment #3
Monochrome CreditAttribution: Monochrome commentedAttached a patch to fix this issue. Adds a conditions to the rule that checks if favicon.ico doesn't actually exist.
Comment #5
Monochrome CreditAttribution: Monochrome commentedComment #6
Monochrome CreditAttribution: Monochrome commented#3: checknofavicon-1041580-3.patch queued for re-testing.
Comment #7
Ali Mirjamali CreditAttribution: Ali Mirjamali commentedTested the above patch with IIS 7 and Drupal 7.15. I confirm that I works. Thanks for the solution.
Comment #8
Ali Mirjamali CreditAttribution: Ali Mirjamali commented#3: checknofavicon-1041580-3.patch queued for re-testing.
Comment #9
RumpledElf CreditAttribution: RumpledElf commentedCan 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 >.<
Comment #10
dcam CreditAttribution: dcam commented#3: checknofavicon-1041580-3.patch queued for re-testing.
Comment #11
dcam CreditAttribution: dcam commentedI 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.
Comment #12
webchickCommitted and pushed to 8.x. Thanks!
This didn't apply cleanly, so markting to be ported for D7.
Suggested commit message:
(daycrom doesn't get picked up by Dreditor, but he made the suggested fix in #2.)
Comment #13
dcam CreditAttribution: dcam commentedBackported #3 to D7.
Comment #15
dcam CreditAttribution: dcam commentedSetting correct version.
Comment #16
dcam CreditAttribution: dcam commented#13: checknofavicon-1041580-13.patch queued for re-testing.
Comment #17
dcam CreditAttribution: dcam commentedI 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.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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.Comment #19
News4u CreditAttribution: News4u commentedthis 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
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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.)