Hello!

On Mon, Jan 23, 2017 at 06:23:05AM +0000, Alon Blayer-Gat wrote:

> Patch attached
>
> On Sun, Jan 22, 2017 at 4:07 PM Alon Blayer-Gat <alon.blayergat@gmail.com>
> wrote:
>
> > Sure. Thanks for the feedback. I made is simpler.
> >
> > evil 'if in location' removed :). I also removed the 'gunzip types;'
> > option.
> > And now we now only have 'gunzip off|on|always'
> >
> > But then again, with 'always', one must specify with 'gunzip_types' the
> > mime types to always gunzip.
> > The rational is that if we want to always gunzip, then it's probably not
> > because the client does not support it but rather because we would like to
> > modify the response. In which case, it is needed only for specific content
> > types.
> > Default mime type is text/html ( similar to gzip_types).
> >
> > I thought of your suggestion of making a more generic mechanism to be used
> > by modules directly. It would require modifications in existing modules to
> > make the feature usable.
> > This patch suggests a simple change for the benefit of existing
> > text-related body-filter modules.
> >
> > Hope it makes sense.
> > Patch attached.
> >
> > Thanks,
> >
> > /Alon
> >
> >
> > On Wed, Dec 7, 2016 at 5:58 PM Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > > Hello!
> > >
> > > On Sun, Nov 27, 2016 at 02:27:56PM +0200, Alon Blayer-Gat wrote:
> > >
> > > > Hi,
> > > >
> > > > 1) 'gunzip always' option will gunzip even if the client supports it.
> > > > 2) 'gunzip types', like 'always' but only for file types specified
> > > > with 'gunzip_types <mime-types>'
> > > > 3) Allow gunzip and gunzip_types directives within "if in location"
> > > > block (rewrite phase condition).
> > > >
> > > > The suggested changes are needed, mainly, to allow dynamic
> > > > modification of compressed response (e.g. with the 'sub_filter'
> > > > module)
> > > > 'types' and 'if in location' may allow a more selective operation.
> > >
> > > No, thanks.
> > >
> > > "If in location" is evil, don't even try to suggest patches to
> > > allow directives in the "if in location" context.
> > >
> > > As for other changes - I can't say I like them as well. We may
> > > consider something as simple as "gunzip always", but additional
> > > types filter certainly looks like an overkill. Rather it should
> > > be some more generic mechanism to require gunzipping, may be
> > > useable by modules directly.

[...]

> @@ -140,13 +161,20 @@
>
> r->gzip_vary = 1;
>
> - if (!r->gzip_tested) {
> - if (ngx_http_gzip_ok(r) == NGX_OK) {
> - return ngx_http_next_header_filter(r);
> + if (conf->enable == NGX_HTTP_GUNZIP_ON) {
> + if (!r->gzip_tested) {
> + if (ngx_http_gzip_ok(r) == NGX_OK) {
> + return ngx_http_next_header_filter(r);
> + }
> +
> + } else if (r->gzip_ok) {
> + return ngx_http_next_header_filter(r);
> }
>
> - } else if (r->gzip_ok) {
> - return ngx_http_next_header_filter(r);
> + } else if (conf->enable == NGX_HTTP_GUNZIP_ALWAYS
> + && ngx_http_test_content_type(r, &conf->types) == NULL)
> + {
> + return ngx_http_next_header_filter(r);
> }

With such a code the behaviour is going to be very inconsistent,
for example, if a client does not support gzip:

- "gunzip on" will gunzip everything if gzip is not supported by
the client;

- "gunzip always" will _not_ gunzip anything not listed in
gunzip_types.

Not to mention that "always" is expected to do it always.
Introducing additional filtering with such a name is likely to
confuse users.

As previously suggested, I would rather consider "gunzip always"
to do exactly what it says, without any filtering.

--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel