Should I send patch here? - xslt_enable "only enable xslt transform for certain user agent"
tOmasEn
tomasen at gmail.com
Wed Jan 27 19:28:24 MSK 2010
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://nginx.org/pipermail/nginx/attachments/20100128/58b5611b/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xslt_enable.patch
Type: application/octet-stream
Size: 3837 bytes
Desc: not available
URL: <http://nginx.org/pipermail/nginx/attachments/20100128/58b5611b/attachment-0001.obj>
More information about the nginx
mailing list