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

Issue parsing single value cache-control directive ending in comma #2918

Closed
ghost opened this issue Sep 3, 2019 · 8 comments · Fixed by #2927
Closed

Issue parsing single value cache-control directive ending in comma #2918

ghost opened this issue Sep 3, 2019 · 8 comments · Fixed by #2927

Comments

@ghost
Copy link

ghost commented Sep 3, 2019

I'm using the .htaccess file configuration suggested by Webhint. The default cache-control for the favicon is 1 hour; why so short a period? Favicons aren't updated frequently. GTMetrix suggests 1 week, so I edited the directive in the .htaccess file to:

<Files "favicon.ico">
	ExpiresByType image/x-icon "access plus 1 week"
</Files>

But then the online scanner (with webhint version: 5.2.2) generates this hint:

hint webhintio/online-service#1: The directive is invalid

https://thomasbrodhead.com/favicons/favicon.20190811163149.ico

Cache-Control: max-age=604800,

...which is 1 week; why is this an error?

@molant
Copy link
Member

molant commented Sep 3, 2019

@bmt-systems the recommended value for resources and max-age is at least one year:

For the resources max-age should be greater or equal to 1 year. You can change this as follows:

@molant
Copy link
Member

molant commented Sep 3, 2019

Moving this to the CLI repo as we have an error in the apache configuration.

@molant molant transferred this issue from webhintio/online-service Sep 3, 2019
@molant molant changed the title [online scanner] Why is "access plus 1 hour" the only acceptable Expires header for favicon? Apache configuration suggests using "access plus 1 hour" for favicon instead of 1 year Sep 3, 2019
@molant molant added this to the 1909-1 milestone Sep 3, 2019
@ghost
Copy link
Author

ghost commented Sep 3, 2019

I changed the directive in my .htaccess file to:

<Files "favicon.ico">
    ExpiresByType image/x-icon    "access plus 1 year"
</Files>

But the online scanner still throws a hint:

hint #1: The directive is invalid

https://thomasbrodhead … avicon.20190811163149.ico

Cache-Control: max-age=31536000,

@molant
Copy link
Member

molant commented Sep 3, 2019

At least we've changed the error message!

I'm thinking the error is the trailing , at the end of the directive. That's probably something we shouldn't flag. What version of apache are you using?

@ghost
Copy link
Author

ghost commented Sep 3, 2019

Apache version 2.4.39

@molant molant self-assigned this Sep 4, 2019
@molant molant changed the title Apache configuration suggests using "access plus 1 hour" for favicon instead of 1 year Issue parsing single value cache-control directive ending in comma Sep 4, 2019
@molant
Copy link
Member

molant commented Sep 4, 2019

I've reviewed the whole Apache configuration in the docs, and it should be ok.

There's the following lines a bit above the favicon.ico one:

# By default, inform user agents to cache all resources for 1 year.

ExpiresDefault                                   "access plus 1 year"

And the one you were modifying is only for files named favicon.ico. You are already revamping that file to favicon.20190811163149.ico so that rule shouldn't get applied in your case.

I'm looking into why webhint reports the directive is invalid.

@molant
Copy link
Member

molant commented Sep 5, 2019

@bmt-systems I've opened #2927 to address the current issue. I've used your website for testing and now it should tell you to add the immutable directive.

@ghost
Copy link
Author

ghost commented Sep 5, 2019

I'm using the browser extension and the online scanner. Please let me know when either is updated with the patch and I'll test on my end.

antross pushed a commit that referenced this issue Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant