Hello!

On Mon, Jan 02, 2012 at 11:24:04PM +0100, SamB wrote:

> Hi,
> I've made a (relatively) simple patch for HttpXsltModule, that adds 2 new
> config directives and slightly modifies the xslt parsing process in
> HttpXsltModule:
>
> xslt_use_root - enables new xslt file location mode. If enabled
> non-absolute filenames are being located relatively to root location (if
> root defined without variables), and not relatively to nginx work
> directory. To achieve this xslt file parsing has been moved from directive
> processing routine, to merge routine and file location has been extended
> accordingly.
> This makes xslt files deploying easier, especially when deploying beta
> files (in testing) to production site. Before it has been necessary to
> modify file paths to match production site config, including modification
> of xslt include/import directives urls, after enabling it, simple file copy
> + nginx restart is enough.

This isn't taking into account root with variables, as well as
alias used instead of root. (And the patch doesn't even provide
any diagnostics for the case when it's not going to work.) Apart
from this, you probably don't want your xslt files to be exposed
under document root for security reasons.

I believe correct solution for the use case in question would be
some form of configuration macros to set both document root and
location of xslt files from the common prefix. Right now it may
be easily done with trivial external text processing.

> xslt_stylesheet_param - this is equivalent of xslt parameter passed to
> xslt_stylesheet, but this works independently and globally. Implemented as
> configuration directive processing, location parameters merging, and
> finally xslt stylesheet parameters merging.
> Again, xslt configuration is a bit simpler because 'common' xslt variables
> don't need to be configured for each xslt_stylesheet, but can be defined
> globally as needed.

This may be intresting, but:

1. One change at a time, please. There is no way to accept/review
changes which are mixed with other changes. And no unrelated
changes, please. If you want change to happen, submit it
explicitly as a separate patch.

2. Please follow coding style. There is a lot of style violations
in your patch.

3. You merge params with ones specified at previous levels. This
is incorrect approach, it should follow the same model as other
array-type variables, i.e. it should be inherited from previous
levels unless there are own params at the level in question.

Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel