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

Server NGINX
1

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.

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?

2

Hi,

tOmasEn wrote:

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?
If you’re just adding new features, you’re probably better off putting
it in a separate module and publishing that.

Marcus.

3

Not entirely a new feature. just give out more control to user for
current
xslt module. about when or when not to use current xslt module.

It doesn’t seems necessarily to me, to fork a new module to publish

4

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 :expressionless: .

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.
5

here is the correct formated(I hope) patch. Sorry for the last one

6

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

7

Hello!

Just a note: it’s probably better idea to post patches to
nginx-devel@. 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
Sina Visitor System

*** …/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.

! 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_string("text/xml"),

! NULL },

! ngx_null_command
};

Unrelated (and wrong) whitespace change here.

  • ngx_http_xslt_enable(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
  • {

Please move config parsing function to other config parsing
functions.

  • 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.

  • for (i = 1; i < cf->args->nelts; i++) {
  •   ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "%V", &rc.err);
  • #else
  • ngx_str_t *value;
  • value = cf->args->elts;

Any reason to use value here? Looks like cut-n-paste leftover
from gzip_disable.

{

*** 299,318 ****

  •   == 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 D.