Fix 'Vary' header #98
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@WhaleHub I was asking myself the same question. I'd like to know when it will be merged by the Google team 🤗 |
| if (cfg->enable == NGX_HTTP_BROTLI_STATIC_ON) { | ||
| req->gzip_vary = 1; | ||
|
|
||
| if (rc != NGX_OK) { |
There was a problem hiding this comment.
rc can not be != NGX_OK here
| } else { | ||
| /* NGX_HTTP_BROTLI_STATIC_ON */ | ||
| req->gzip_vary = 1; | ||
| if (cfg->enable == NGX_HTTP_BROTLI_STATIC_ON) { |
There was a problem hiding this comment.
It is preferrable to have least-modification concise change. Just remove req->gzip_vary = 1 line
| @@ -227,6 +227,14 @@ static ngx_int_t handler(ngx_http_request_t* req) { | |||
| return NGX_DECLINED; | |||
| } | |||
|
|
|||
| if (cfg->enable == NGX_HTTP_BROTLI_STATIC_ON) { | |||
There was a problem hiding this comment.
Not sure if this should be conditional here.
General rule - if compressed entry is going to be server - it must have vary header.
Syncing with master
Here is a patch regarding the issue I recently posted: #97 I used the nginx gzip_static module as a comparison since I know it works as-expected. I've tested this on my dev server and happy to report that it is no longer setting the header for images. It is correctly dynamically compressing content, and also correctly sending pre-compressed static files.
The only thing I'm not sure about is under 'NGX_HTTP_BROTLI_STATIC_ALWAYS', there's just the
/* always */comment. In the gzip_static module there is alsorc = NGX_OK;which I'm not 100% sure if it should be there or not. I left it out (since I don't use thealwaysoption). Maybe it should be there?