Should I send patch here? - xslt_enable "only enable xslt transform for certain user agent"

tOmasEn tomasen at gmail.com
Wed Jan 27 20:12:28 MSK 2010


a little more change:
make "xslt_enable" work at server level also "xslt_stylesheet"

On Thu, Jan 28, 2010 at 12:28 AM, tOmasEn <tomasen at gmail.com> wrote:

> here is the correct formated(I hope) patch. Sorry for the last one
>
>
> On Thu, Jan 28, 2010 at 12:03 AM, tOmasEn <tomasen at gmail.com> wrote:
>
>> Thanks a lot for reply. I will try make the adjustments accordingly.
>>
>>
>> > Any reason to use value here?  Looks like cut-n-paste leftover
>> > from gzip_disable.
>>
>> Yes, I did copy those from gzip_disable. be frankly, I'm still not very
>> clear on the correct ways to use ngx_buf ,  ngx_string , ngx_regex . Also
>> not sure where to look except code sample from other module :| .
>>
>> For example. I'm thinking to detect if the xml do fit for xslt transform
>> before put it though, which mean the xml should have line like
>> <?xml-stylesheet type="text/xsl" href="xxxxx"/> . I guess I should put it at
>> beginning of ngx_http_xslt_body_filter. and check if I can find
>> "xml-stylesheet" in  (ngx_chain_t *)in->buf. But not sure what's the correct
>> way to handle ngx_buf .
>>
>> I'll subscribe nginx-devel now.
>>
>>
>>
>> On Wed, Jan 27, 2010 at 8:49 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
>>
>>> Hello!
>>>
>>> Just a note: it's probably better idea to post patches to
>>> nginx-devel at .  Cc'd.
>>>
>>> On Wed, Jan 27, 2010 at 06:04:16PM +0800, tOmasEn wrote:
>>>
>>> > Maybe not many people using xslt as main web layout. but I do.
>>> >
>>> > This patch is similar to gzip_disable.  xslt_enable only enable xslt
>>> > transform for certain user agent.
>>> >
>>> > e.g.
>>> > xslt_enable "Googlebot"
>>> > will only enable xslt transform output for google's crawler.
>>>
>>> While I tend to think this is good feature (though not sure), I
>>> also think that it probably needs some better name.  Something
>>> more self-explaining.
>>>
>>> Also it's probably better to make xslt controllable via variable
>>> instead.  This will handle you "disable via special header" case
>>> as well.
>>>
>>> > And, I'm also planing some other improve on xslt module. Like: when
>>> xslt
>>> > transform failed, output original xml file instead of 500 server error;
>>> Skip
>>> > xslt transform if client send specified HTTP header like "No-XSLT",
>>> etc.
>>> > Should I talk to somebody and discuss the architect?
>>> >
>>> > --
>>> > Tomasen
>>> > http://twitter.com/ShooterPlayer
>>> > http://t.sina.com.cn/Tomasen
>>>
>>> > *** ../nginx-0.8.31/src/http/modules/ngx_http_xslt_filter_module.c
>>>  2010-01-27 17:23:37.000000000 +0800
>>> > --- nginx-0.8.31/src/http/modules/ngx_http_xslt_filter_module.c
>>> 2009-11-30 21:15:10.000000000 +0800
>>> > ***************
>>> > *** 48,54 ****
>>>
>>> Please use unified diff patches, and not reversed ones.
>>>
>>> >       ngx_array_t          sheets;       /* ngx_http_xslt_sheet_t */
>>> >       ngx_hash_t           types;
>>> >       ngx_array_t         *types_keys;
>>> > -     ngx_array_t         *xslt_enable;            /* xslt_enable */
>>>
>>> Comment is pointless and wrongly placed.  Just remove it.
>>>
>>> >   } ngx_http_xslt_filter_loc_conf_t;
>>> >
>>> >
>>> > --- 48,53 ----
>>> > ***************
>>> > *** 135,143 ****
>>> >       void *child);
>>> >   static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);
>>> >   static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);
>>> > !
>>> > ! static char *ngx_http_xslt_enable(ngx_conf_t *cf, ngx_command_t *cmd,
>>> > !                                                                void
>>> *conf);
>>>
>>> 1. Please follow nginx style.
>>>
>>> 2. Please move this to other config parsing functions.
>>>
>>> >
>>> >   ngx_str_t  ngx_http_xslt_default_types[] = {
>>> >       ngx_string("text/xml"),
>>> > --- 134,140 ----
>>> >       void *child);
>>> >   static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);
>>> >   static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);
>>> > !
>>> >
>>> >   ngx_str_t  ngx_http_xslt_default_types[] = {
>>> >       ngx_string("text/xml"),
>>> > ***************
>>> > *** 168,183 ****
>>> >         offsetof(ngx_http_xslt_filter_loc_conf_t, types_keys),
>>> >         &ngx_http_xslt_default_types[0] },
>>> >
>>> > !     { ngx_string("xslt_enable"),
>>> > !
>>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,
>>> > !       ngx_http_xslt_enable,
>>> > !       NGX_HTTP_LOC_CONF_OFFSET,
>>> > !       offsetof(ngx_http_xslt_filter_loc_conf_t, xslt_enable),
>>> > !       NULL },
>>> > !
>>> > !     ngx_null_command
>>> >   };
>>> >
>>> >   static ngx_http_module_t  ngx_http_xslt_filter_module_ctx = {
>>> >       NULL,                                  /* preconfiguration */
>>> >       ngx_http_xslt_filter_init,             /* postconfiguration */
>>> > --- 165,174 ----
>>> >         offsetof(ngx_http_xslt_filter_loc_conf_t, types_keys),
>>> >         &ngx_http_xslt_default_types[0] },
>>> >
>>> > !       ngx_null_command
>>> >   };
>>> >
>>> > +
>>>
>>> Unrelated (and wrong) whitespace change here.
>>>
>>> >   static ngx_http_module_t  ngx_http_xslt_filter_module_ctx = {
>>> >       NULL,                                  /* preconfiguration */
>>> >       ngx_http_xslt_filter_init,             /* postconfiguration */
>>> > ***************
>>> > *** 212,289 ****
>>> >   static ngx_http_output_header_filter_pt  ngx_http_next_header_filter;
>>> >   static ngx_http_output_body_filter_pt    ngx_http_next_body_filter;
>>> >
>>> > -
>>> > - static char *
>>> > - ngx_http_xslt_enable(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
>>> > - {
>>>
>>> Please move config parsing function to other config parsing
>>> functions.
>>>
>>> > -     ngx_http_xslt_filter_loc_conf_t  *clcf = conf;
>>> > -
>>> > - #if (NGX_PCRE)
>>> > -     ngx_str_t            *value;
>>> > -     ngx_uint_t            i;
>>> > -     ngx_regex_elt_t      *re;
>>> > -     ngx_regex_compile_t   rc;
>>> > -     u_char                errstr[NGX_MAX_CONF_ERRSTR];
>>> > -
>>> > -     if (clcf->xslt_enable == NULL) {
>>> > -             clcf->xslt_enable = ngx_array_create(cf->pool, 2,
>>> > -                     sizeof(ngx_regex_elt_t));
>>> > -             if (clcf->xslt_enable == NULL) {
>>> > -                     return NGX_CONF_ERROR;
>>> > -             }
>>>
>>> No tabs, please.
>>>
>>> > -     }else{
>>> > -             //xslt_enable_is_not_required
>>> > -             return NGX_CONF_OK;
>>>
>>> 1. No C++ comments, please.
>>>
>>> 2. You should either return error here (something like return "is
>>> duplicate") or append to list as gzip_disable do.  Blindly
>>> ignoring configuration directive isn't what nginx generally do.
>>>
>>> > -     }
>>> > -
>>> > -     value = cf->args->elts;
>>> > -
>>> > -     ngx_memzero(&rc, sizeof(ngx_regex_compile_t));
>>> > -
>>> > -     rc.pool = cf->pool;
>>> > -     rc.err.len = NGX_MAX_CONF_ERRSTR;
>>> > -     rc.err.data = errstr;
>>> > -
>>> > -     for (i = 1; i < cf->args->nelts; i++) {
>>> > -
>>> > -             re = ngx_array_push(clcf->xslt_enable);
>>> > -             if (re == NULL) {
>>> > -                     return NGX_CONF_ERROR;
>>> > -             }
>>> > -
>>> > -             rc.pattern = value[1];
>>> > -             rc.options = NGX_REGEX_CASELESS;
>>> > -
>>> > -             if (ngx_regex_compile(&rc) != NGX_OK) {
>>> > -                     ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "%V",
>>> &rc.err);
>>> > -                     return NGX_CONF_ERROR;
>>> > -             }
>>> > -
>>> > -             re->regex = rc.regex;
>>> > -             re->name = value[i].data;
>>> > -
>>> > -     }
>>> > -
>>> > -     return NGX_CONF_OK;
>>> > -
>>> > - #else
>>> > -     ngx_str_t  *value;
>>> > -
>>> > -     value = cf->args->elts;
>>>
>>> Any reason to use value here?  Looks like cut-n-paste leftover
>>> from gzip_disable.
>>>
>>> > -
>>> > -     ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
>>> > -             "without PCRE library \"xslt_enable\" won't work ");
>>> > -
>>> > -     return NGX_CONF_ERROR;
>>> > - #endif
>>> > - }
>>> >
>>> >   static ngx_int_t
>>> >   ngx_http_xslt_header_filter(ngx_http_request_t *r)
>>> >   {
>>> >       ngx_http_xslt_filter_ctx_t       *ctx;
>>> >       ngx_http_xslt_filter_loc_conf_t  *conf;
>>> > !
>>> >       ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>>> >                      "xslt filter header");
>>> >
>>> > --- 203,215 ----
>>> >   static ngx_http_output_header_filter_pt  ngx_http_next_header_filter;
>>> >   static ngx_http_output_body_filter_pt    ngx_http_next_body_filter;
>>> >
>>> >
>>> >   static ngx_int_t
>>> >   ngx_http_xslt_header_filter(ngx_http_request_t *r)
>>> >   {
>>> >       ngx_http_xslt_filter_ctx_t       *ctx;
>>> >       ngx_http_xslt_filter_loc_conf_t  *conf;
>>> > !
>>> >       ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>>> >                      "xslt filter header");
>>> >
>>> > ***************
>>> > *** 299,318 ****
>>> >           return ngx_http_next_header_filter(r);
>>> >       }
>>> >
>>> > - #if (NGX_PCRE)
>>> > -
>>> > -     if (conf->xslt_enable && r->headers_in.user_agent) {
>>> > -
>>> > -             if (ngx_regex_exec_array(conf->xslt_enable,
>>> > -                     &r->headers_in.user_agent->value,
>>> > -                     r->connection->log)
>>> > -                     == NGX_DECLINED)
>>> > -             {
>>> > -                     return ngx_http_next_header_filter(r);
>>> > -             }
>>> > -     }
>>> > -
>>> > - #endif
>>> >       ctx = ngx_http_get_module_ctx(r, ngx_http_xslt_filter_module);
>>> >
>>> >       if (ctx) {
>>> > --- 225,230 ----
>>>
>>> Maxim Dounin
>>>
>>> _______________________________________________
>>> nginx mailing list
>>> nginx at nginx.org
>>> http://nginx.org/mailman/listinfo/nginx
>>>
>>
>>
>>
>> --
>> Tomasen
>> http://twitter.com/ShooterPlayer
>> http://t.sina.com.cn/Tomasen
>>
>
>
>
> --
> Tomasen
> http://twitter.com/ShooterPlayer
> http://t.sina.com.cn/Tomasen
>



-- 
Tomasen
http://twitter.com/ShooterPlayer
http://t.sina.com.cn/Tomasen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://nginx.org/pipermail/nginx-devel/attachments/20100128/dceb86ed/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xslt_enable.patch
Type: application/octet-stream
Size: 4679 bytes
Desc: not available
URL: <http://nginx.org/pipermail/nginx-devel/attachments/20100128/dceb86ed/attachment-0001.obj>


More information about the nginx-devel mailing list