Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.2][fix] Return proper 404 for missing favicon.ico, missing images etc. #530

Closed
vbmark opened this issue Dec 17, 2014 · 9 comments
Closed

Comments

@vbmark
Copy link

vbmark commented Dec 17, 2014

I set a breakpoint in libs\Application.php in __construct() and found it was getting hit twice.

The second hit is a get of favicon.ico and it was not found so it looks like there is a behind-the-scenes redirect to the error page happening. However, there is no visual effect on the rendered page view.

This seems like extra and unnecessary work going on.

What I did was under this line:

if ($this->url_controller) {

I added this:

if ($this->url_controller == 'favicon.ico') {
return;
}

I don't know if that's the best way but it seems to be working for me.

@panique
Copy link
Owner

panique commented Dec 17, 2014

Hmm, are you sure ? A favicon.icon is a static file and has nothing to do with the script itself. If you are correct, then this would affect every framework in the PHP world, as the mod_rewrite rule used here is exactly the same like in most others that don't put index.php in /public folder.

Can anybody confirm this ?
@NeoN5 Can you post your .htaccess file please ? Maybe it's different.

@Melbournite
Copy link

I've experienced this before when using nginx and the error page is set to redirect to the index. Basically any 404 would result in another hit on the dynamic page, favicon and touch icons included I.e:

error_page 404 /index.php;

I've just tested this now with nginx, and it's not returning multiple hits where no favicon exists, though it does when 404 is redirected to the index. Which strangely is a pretty common practice, though not a fault in the script. I can't speak for Apache (I don't use it anymore). I could only emulate it in IE, though I've seen it across all browsers.

@panique
Copy link
Owner

panique commented Dec 17, 2014

Okay, I've nearly no experiences with nginx, so I cannot say anything here. Can anybody give a workaround for nginx ?

@Melbournite
Copy link

The work around is basically don't redirect 404 to index. It's not an error with the script, it's simply bad practices spreading to common usage. Perhaps redirected 404's results in better user engagement or something, I am not sure.

You could, in theory, return a 204 for favicon:
location = /favicon.ico {
return 204;
}

Of course, not suggesting that @NeoN5 is experiencing this problem due to that. But thought it would be worthy to mention. Perhaps we should let @NeoN5 elaborate before we jump to any further conclusions :P

@panique
Copy link
Owner

panique commented Dec 17, 2014

By the way, interesting side-fact: SensioLabs's php-project-code-checker-tool (https://insight.sensiolabs.com/) gives back a big error when you analyze a PHP project that has no favicon, which is weird and always bugged me as the favicon has absolutly nothing to do with a PHP-codebase.

Maybe THAT's the reason. :)

@vbmark
Copy link
Author

vbmark commented Dec 17, 2014

@panique I've not even looked at the .htaccess file much less changed it.

Yes, I'm sure it's happening. Breakpoint is hit twice and on the second hit $this->url_controller == 'favicon.ico'.

Something of note, though. The second hit is not instant. It's like there is a 4 second delay before the call is made. Could it be the Chrome browser making the call?

@Melbournite
Copy link

@NeoN5 so what happens if you go to http://domain/favicon.ico in Chrome?

@panique panique changed the title favicon.ico get causing extra processing [2.1][fix][nginx] favicon.ico get causing extra processing in nginx Jan 20, 2015
@panique panique changed the title [2.1][fix][nginx] favicon.ico get causing extra processing in nginx [2.1][fix] favicon.ico get causing extra processing Jan 20, 2015
@panique panique changed the title [2.1][fix] favicon.ico get causing extra processing [3.0][fix] favicon.ico get causing extra processing Jan 25, 2015
@panique panique added the v3.0 label Jan 25, 2015
@panique panique changed the title [3.0][fix] favicon.ico get causing extra processing [3.2][fix] Return proper 404 for missing favicon.ico, missing images etc. Mar 9, 2015
@panique
Copy link
Owner

panique commented Mar 9, 2015

I think this could / should be fixed within the .htaccess, as the browser tries to call a static file, so this could be catched outside of the php application.

panique added a commit that referenced this issue Jul 9, 2015
…ty image as fallback to prevent server from getting 404-hammered)
@panique
Copy link
Owner

panique commented Jul 9, 2015

The project now has a favicon and also a nice "empty-image"-fallback in the HTML header.

@panique panique closed this as completed Jul 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants