Hello,

I've modified my patch, according to comments in
http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001751.html.

Directive has been renamed to xslt_param, taking 2 parameters - name and
value separated (similarly to set directive),
therefore it's now also possible to pass ':' character as it had been
requested before.
I think it's better and easier to have it this way + there is no need to
do that ugly parsing in runtime ;)

Thanks
Sam

On Tue, Jan 31, 2012 at 1:00 PM, <nginx-devel-request@nginx.org> wrote:

>
> Message: 6
> Date: Tue, 31 Jan 2012 05:52:23 +0400
> From: Maxim Dounin <mdounin@mdounin.ru>
> To: nginx-devel@nginx.org
> Subject: Re: [PATCH] add xslt_stylesheet_param directive
> Message-ID: <20120131015222.GH67687@mdounin.ru>
> Content-Type: text/plain; charset=us-ascii
>
> Hello!
>
> On Tue, Jan 24, 2012 at 11:02:13PM +0100, SamB wrote:
>
> > Hi,
> > next try...
> >
> > Attached patch adds xslt_stylesheet_param directive processing.
> > This is equivalent of xslt parameter passed to xslt_stylesheet, but
> this
> > works independently and globally.
> > xslt config should be a bit simpler because 'common' xslt variables
> > don't need to be repeated for each xslt_stylesheet.
> >
> > Comments/suggestion are welcomed.
>
> You may have better lock pointing to a previous incarnation of the
> patch and outlining changes made. (And please post in plain text,
> not html.)
>
> It was previously submitted here:
>
> http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001669.html
>
> And previous review was here:
>
> http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001670.html
>
>
> > diff -ur nginx-1.1.13/src/http/modules/ngx_http_xslt_filter_module.c
> nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c
> > --- nginx-1.1.13/src/http/modules/ngx_http_xslt_filter_module.c
> 2010-07-12 14:52:01.000000000 +0200
> > +++ nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c
> 2012-01-24 22:33:24.000000000 +0100
> > @@ -48,6 +48,7 @@
> > ngx_array_t sheets; /* ngx_http_xslt_sheet_t */
> > ngx_hash_t types;
> > ngx_array_t *types_keys;
> > + ngx_array_t params; /* ngx_http_complex_value_t */
> > } ngx_http_xslt_filter_loc_conf_t;
> >
> >
> > @@ -84,12 +85,16 @@
> > void *conf);
> > static char *ngx_http_xslt_stylesheet(ngx_conf_t *cf, ngx_command_t
> *cmd,
> > void *conf);
> > +static char *ngx_http_xslt_stylesheet_param(ngx_conf_t *cf,
> ngx_command_t *cmd,
> > + void *conf);
> > static void ngx_http_xslt_cleanup_dtd(void *data);
> > static void ngx_http_xslt_cleanup_stylesheet(void *data);
> > static void *ngx_http_xslt_filter_create_main_conf(ngx_conf_t *cf);
> > static void *ngx_http_xslt_filter_create_conf(ngx_conf_t *cf);
> > +static char *ngx_http_xslt_filter_merge_params(const ngx_array_t *src,
> ngx_array_t *dest);
> > static char *ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void
> *parent,
> > void *child);
> > +static ngx_int_t ngx_http_xslt_filter_preinit(ngx_conf_t *cf);
> > static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);
> > static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);
> >
> > @@ -116,6 +121,13 @@
> > 0,
> > NULL },
> >
> > + { ngx_string("xslt_stylesheet_param"),
> > +
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> > + ngx_http_xslt_stylesheet_param,
> > + NGX_HTTP_LOC_CONF_OFFSET,
> > + 0,
> > + NULL },
>
> I tend to think that "xslt_stylesheet_param" is too long.
> Something like "xslt_param" whould be better.
>
> > +
> > { ngx_string("xslt_types"),
> >
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,
> > ngx_http_types_slot,
> > @@ -128,7 +140,7 @@
> >
> >
> > static ngx_http_module_t ngx_http_xslt_filter_module_ctx = {
> > - NULL, /* preconfiguration */
> > + ngx_http_xslt_filter_preinit, /* preconfiguration */
>
> The "...preinit" change is unrelated. If you want it to happen -
> please submit it seprately. (I think it's noop though.)
>
> > ngx_http_xslt_filter_init, /* postconfiguration */
> >
> > ngx_http_xslt_filter_create_main_conf, /* create main configuration
> */
> > @@ -865,6 +877,46 @@
> > }
> >
> >
> > +static char *
> > +ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd, void
> *conf)
> > +{
> > + ngx_http_xslt_filter_loc_conf_t *xlcf = conf;
> > +
> > + ngx_http_complex_value_t *param;
> > + ngx_http_compile_complex_value_t ccv;
> > + ngx_str_t *value;
> > +
> > + value = cf->args->elts;
> > +
> > + /* alloc array */
> > + if (xlcf->params.elts == NULL
> > + && ngx_array_init(&xlcf->params, cf->pool, 1,
> sizeof(ngx_http_complex_value_t))
> > + != NGX_OK)
> > + {
> > + return NGX_CONF_ERROR;
> > + }
>
> No lines longer than 80 chars, please. And alignment isn't
> styl-complaint either. In this particular case, please just use
> another nested if, much like in ngx_http_xslt_stylesheet():
>
> if (xlcf->sheets.elts == NULL) {
> if (ngx_array_init(&xlcf->sheets, cf->pool, 1,
> sizeof(ngx_http_xslt_sheet_t))
> != NGX_OK)
> {
> return NGX_CONF_ERROR;
> }
> }
>
> > +
> > + param = ngx_array_push(&xlcf->params);
> > + if (param == NULL) {
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
> > +
> > + ccv.cf = cf;
> > + ccv.value = &value[1];
> > + ccv.complex_value = param;
> > + ccv.zero = 1;
> > +
> > + if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + return NGX_CONF_OK;
> > +}
> > +
> > +
> > +
>
> 2 blank lines between functions, please. 3 blank lines before
> ngx_http_xslt_stylesheet() is a bug, not a rule.
>
> Another question to consider: there was at least one request to
> simplify passing XPath expressions in params by allowing ':', see
> here:
>
> http://mailman.nginx.org/pipermail/nginx-devel/2010-July/000414.html
> http://mailman.nginx.org/pipermail/nginx-devel/2010-September/000486.html
>
> With separate directive we may want to allow this. Not sure
> though, probably it would be better to preserve compatibility with
> a way how parameters in xslt_stylesheet directive are handled.
>
> > static void
> > ngx_http_xslt_cleanup_dtd(void *data)
> > {
> > @@ -931,6 +983,37 @@
> >
> >
> > static char *
> > +ngx_http_xslt_filter_merge_params(const ngx_array_t *src, ngx_array_t
> *dest)
> > +{
> > + ngx_uint_t i;
> > +
> > + /* copy params */
> > + if (src->nelts && !dest->nelts) {
> > + dest->elts = src->elts;
> > + dest->nelts = src->nelts;
> > + }
> > + /* join params */
> > + else if (src->nelts && dest->nelts) {
> > + ngx_http_complex_value_t *params;
> > +
> > + params = src->elts;
> > + for (i = 0; i < src->nelts; i++) {
> > + ngx_http_complex_value_t *new;
> > +
> > + new = ngx_array_push(dest);
> > + if (new == NULL) {
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + *new = params[i]; /* copy */
> > + }
> > + }
> > +
> > + return NGX_CONF_OK;
> > +}
> > +
> > +
> > +static char *
> > ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void *parent, void
> *child)
> > {
> > ngx_http_xslt_filter_loc_conf_t *prev = parent;
> > @@ -940,9 +1023,26 @@
> > conf->dtd = prev->dtd;
> > }
> >
> > + if (ngx_http_xslt_filter_merge_params(&prev->params, &conf->params)
> > + != NGX_CONF_OK) {
> > + return NGX_CONF_ERROR;
> > + }
>
> As I already said in the previous review, params should follow the
> same logic as other array-type directives (access_log,
> proxy_set_header, fastcgi_param, error_page, ...). I.e. if you
> define any directive on a particular level, previous level data
> should not be inherited:
>
> location / {
> xslt_param foo=1;
>
> location /bar/ {
> xslt_param bar=1;
> }
> }
>
> In location /bar/ only "bar=1" parameter should be present.
>
> Please also note that with your code it's not possible to clear
> inherited params at all.
>
> > +
> > if (conf->sheets.nelts == 0) {
> > conf->sheets = prev->sheets;
> > }
> > + else {
> > + ngx_http_xslt_sheet_t *sheet;
> > + ngx_uint_t i;
> > +
> > + sheet = conf->sheets.elts;
> > + for (i = 0; i < conf->sheets.nelts; i++) {
> > + if (ngx_http_xslt_filter_merge_params(&conf->params,
> &sheet[i].params)
> > + != NGX_CONF_OK) {
> > + return NGX_CONF_ERROR;
> > + }
> > + }
> > + }
>
> This is wrong. Consider configuration like this:
>
> location / {
> xslt_stylesheet root.xslt;
>
> location /foo/ {
> xslt_params foo=bar;
> }
> }
>
> With the above merge logic in location /foo/ the root.xslt
> stylesheet will be used without parameters, while "foo=bar" is
> clearly expected.
>
> I suggest to use simple array-type inheritance for params, like
>
> if (conf->params == NULL) {
> conf->params = prev->params;
> }
>
> and apply them separately to the stylesheet at runtime. This will
> resolve both problems outlined above.
>
> >
> > if (ngx_http_merge_types(cf, &conf->types_keys, &conf->types,
> > &prev->types_keys, &prev->types,
> > @@ -957,7 +1057,7 @@
> >
> >
> > static ngx_int_t
> > -ngx_http_xslt_filter_init(ngx_conf_t *cf)
> > +ngx_http_xslt_filter_preinit(ngx_conf_t *cf)
> > {
> > xmlInitParser();
> >
> > @@ -965,6 +1065,13 @@
> > exsltRegisterAll();
> > #endif
> >
> > + return NGX_OK;
> > +}
> > +
> > +
> > +static ngx_int_t
> > +ngx_http_xslt_filter_init(ngx_conf_t *cf)
> > +{
> > ngx_http_next_header_filter = ngx_http_top_header_filter;
> > ngx_http_top_header_filter = ngx_http_xslt_header_filter;
> >
>
> See above, this is unrelated change.
>
> Maxim Dounin
>
>
>
> ------------------------------
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> End of nginx-devel Digest, Vol 27, Issue 26
> *******************************************
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel