[PATCH] add xslt_stylesheet_param directive

[Maxim Dounin]

Hello!

On Wed, Feb 08, 2012 at 01:55:47PM +0100, SamB wrote:

> Hi,
> 
>    I've added xslt_string_param and slightly changed parameter passing and
> transformation
>    to make it work as expected.
>    As a side effect of this change, might be usage of
> xslt_param/xslt_string_param faster,
>    because params are directly inserted to transformation context and do
> not need to be
>    copied to name/value parameter array that is passed to
> xsltApplyStylesheet() as it's now
>    by parameters passed via xslt_stylesheet directive.
> 
>    Old parameter passing using xslt_stylesheet has not been changed to
> prevent unexpected behavior,
>    maybe this can be modified later if this patch will prove to be stable...

This looks intresting, though may be it would be better to finish 
and commit the xslt_param patch first (see another message for a 
review).

Below is review of the xslt_param + xslt_string_param patch 
specific bits.

[...]

> 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-02-08 13:34:43.000000000 +0100
> @@ -10,6 +10,7 @@
>  
>  #include <libxml/parser.h>
>  #include <libxml/tree.h>
> +#include <libxslt/variables.h>
>  #include <libxslt/xslt.h>
>  #include <libxslt/xsltInternals.h>
>  #include <libxslt/transform.h>
> @@ -48,6 +49,8 @@
>      ngx_array_t          sheets;       /* ngx_http_xslt_sheet_t */
>      ngx_hash_t           types;
>      ngx_array_t         *types_keys;
> +    ngx_array_t          string_params; /* ngx_http_complex_value_t */
> +    ngx_array_t          params;       /* ngx_http_complex_value_t */

I would like to see single "params" array instead, with both 
string params and xpath ones.

[...]

> @@ -84,6 +90,13 @@
>      void *conf);
>  static char *ngx_http_xslt_stylesheet(ngx_conf_t *cf, ngx_command_t *cmd,
>      void *conf);
> +static char *ngx_http_xslt_add_param(ngx_conf_t *cf, ngx_command_t *cmd,
> +    ngx_array_t *params);
> +static char *ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd,
> +    void *conf);
> +static char *ngx_http_xslt_stylesheet_string_param(ngx_conf_t *cf,
> +    ngx_command_t *cmd,
> +    void *conf);

There is no need for 3 functions here.  Just one is enough.

In your current code, you may (and probably have to) use "offset" 
field of the command structure to pass offset of array you want to 
work with.  With single params array as suggested earlier - you 
may use "post" field to distinguish string and non-string 
versions.

>  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);
> @@ -116,6 +129,20 @@
>        0,
>        NULL },
>  
> +    { ngx_string("xslt_param"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      ngx_http_xslt_stylesheet_param,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      0,
> +      NULL },
> +
> +    { ngx_string("xslt_string_param"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      ngx_http_xslt_stylesheet_string_param,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      0,
> +      NULL },
> +
>      { ngx_string("xslt_types"),
>        NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,
>        ngx_http_types_slot,
> @@ -450,6 +477,7 @@
>      ngx_uint_t                        i;
>      xmlChar                          *buf;
>      xmlDocPtr                         doc, res;
> +    xsltTransformContextPtr           xctxt;

In libxslt this is usually called "ctxt", I think we should 
follow.

>      ngx_http_xslt_sheet_t            *sheet;
>      ngx_http_xslt_filter_loc_conf_t  *conf;
>  
> @@ -468,14 +496,28 @@
>  
>      for (i = 0; i < conf->sheets.nelts; i++) {
>  
> -        if (ngx_http_xslt_params(r, ctx, &sheet[i].params) != NGX_OK) {
> +        xctxt = xsltNewTransformContext(sheet[i].stylesheet, doc);
> +        if (xctxt == NULL) {
>              xmlFreeDoc(doc);
>              return NULL;
>          }
>  
> -        res = xsltApplyStylesheet(sheet[i].stylesheet, doc, ctx->params.elts);
> +        xctxt->initialContextDoc = doc;
> +        xctxt->initialContextNode = (xmlNodePtr) doc;

According to

http://xmlsoft.org/XSLT/html/libxslt-xsltInternals.html

this is internal to libxslt, and using it may not be a good idea.  
Are you sure it's needed?

As far as I can see in sources, it's ceratainly *not* needed for 
xsltQuoteOneUserParam().  Not sure about xsltEvalOneUserParam().

> +
> +        if (ngx_http_xslt_params(r, ctx, conf, xctxt,
> +                                 &sheet[i].params) != NGX_OK) {
> +            xmlFreeDoc(doc);
> +            xsltFreeTransformContext(xctxt);

It may be a good idea to free this in reverse order, i.e. 
transformation context first.

> +            return NULL;
> +        }
> +
> +        res = xsltApplyStylesheetUser(sheet[i].stylesheet, doc,
> +                                      ctx->params.elts,
> +                                      NULL, NULL, xctxt);
>  
>          xmlFreeDoc(doc);
> +        xsltFreeTransformContext(xctxt);

Same here.

>  
>          if (res == NULL) {
>              ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> @@ -564,14 +606,92 @@

[...]

Maxim Dounin