Kudos for splitting code into smaller patches! But I don't think it makes sense to add code to Koha before it is used somewhere - the bigger project might get abandoned and we are left with dead code. I'm setting this to "In discussion" and we can set it back to "Needs signoff" when the functionality that builds on this also needs signoff.

(In reply to Magnus Enger from comment #3)
> Kudos for splitting code into smaller patches! But I don't think it makes
> sense to add code to Koha before it is used somewhere - the bigger project
> might get abandoned and we are left with dead code. I'm setting this to "In
> discussion" and we can set it back to "Needs signoff" when the functionality
> that builds on this also needs signoff.

Back to Needs Signoff. Please look at the dependency. (Bug 11915.) You are most welcome to sign off :)

Marking as candidate for 3.16.0

This looks promising for 3.18, but upon reflection, this has no urgent reason to bypass QA and go into 3.16.0, as there's no user-visible behavior change.  Consequently, I'm removing the rel_3_16_candidate keyword.

Appologies, I've not quite got round to this one. I'm happy with pretty much all of it, but have some last tests that I wan't to run with regards to the remotely hosted sheets as you've removed the getURI routine (Which is great as I do the same in Bug 12172 ).

I 'think' it should now work ok, but it's worth testing, especially with https et al'.

OK, Happy that remotely hosted xslt's work as expected, if you get your sheets right in the first place.  However, if the fetch fails, the relevant page displays without any styling applied and I couldn't find any errors reported anywhere; it would be good to put a warn in somewhere so administrators can track down the problem more easily.

Now I'm going to test the 'path' version of that preference.

Ok, also looks like the pref is still relative to root in the overall filetree and not the root htdocs location, and so this is not a regression.

This should be slightly better documented somewhere me thinks.

Created attachment 28093 [details] [review]
[PASSED QA] Bug 11826: Add XSLT handler object to Koha namespace

In achieving the goals of umbrella report 6536 (Z3950 improvements), this
report adds a XSLT handler object to the Koha namespace.
A follow-up adds unit tests for this module.
This patch does not yet incorporate the new object into Z3950 search. So
it does not change any behavior and is therefore harder to test.

Test plan:
Run the unit tests of the follow-up patch on report 11826.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I tested the new module in two additional ways:

[1] Made a simple script (four lines as below) that ran a larger xml file
    with 1500 records through a test XSLT file modifying the title.

    my $eng= Koha::XSLT_Handler->new;
    open my $fh1, '<', '_example_file';
    my @xml= <$fh1>;
    print $eng->transform( join('', @xml), '_example_xsl_file');

[2] Incorporated the object into my local changes of Breeding.pm (based on
    older work, not ready for submittal now).
    Ran transformation on Z3950 search results. Worked well :)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test runs Ok. No koha-qa errors.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 28094 [details] [review]
[PASSED QA] Bug 11826: Add unit tests for Koha::XSLT_Handler

Test plan:
Verify if XSLT_Handler.t passes.
You could also sabotage the test by removing one of the test xsl files.
Or you could 'repair' the bad xsl file (test02). Remove the second line
redefining the xsl variable.
In all those cases the unit test should fail now.. Discard your changes :)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass. No koha-qa errors.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 28095 [details] [review]
[PASSED QA] Bug 11826: Using the XSLT handler in C4/XSLT module

Incorporate the new object into C4/XSLT module.
The handler object needed one adjustment to allow for passing URLs.
Removed the GetURI function; it is no longer needed.
Added some documentation lines.
Moved a file-level lexical upwards in the code for visibility.
Removed some tabs (on two lines).

Note: The handler could perhaps be moved up to the Context module, or be saved
in a global ('our') variable.  But I would rather not do that now, making
testing not too complex at this moment.

Test plan:
Enable XSLT preferences for opac and intranet.
Check result and detail pages.
Check individual list display (virtual shelves).
Force an XSLT error by editing one of your xsl files. Check the
corresponding display.
Put an URL in one the XSLT prefs (could just be to your own server).
Check the display.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Running XSLTParse4Display in a loop showed no significant performance change.
One pass cost me around 0.012 sec (except for the first pass).

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Results, detail, url and list view Ok
No koha-qa errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 28096 [details] [review]
[PASSED QA] Bug 11826: Use xslt handler object in showmarc, Record.pm

Modifies showmarc and opac-showmarc to use new xslt handler.
Removes cardview.pl as obsolete script.
Modifies C4/Record.pm and a typo in the test Record.t.

Test plan:
[1] catalogue/showmarc: Go to Cataloging. Search. Click Card.
[2] opac-showmarc: Go to opac detail, MARC view.
    Open URL for plain view in new tab.
    Change URL: Change viewas=html to viewas=card
[3] Verify that there are no references in the codebase to cardview.pl
[4] C4/Record.pm: Run the Record.t test in db_dependent.
    This test uses marc2modsxml, triggering the change.
    Additional: export to MODS from opac-detail.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Views Ok. Test pass. No more cardview. No koha-qa errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Passing QA with the following comments:

1. This patch is a good start to tidying up the cslt handling in koha, good work.
2. I find this patch also fixes the https issues in bug 12172
3. I believe this enforces a more strict behaviour when using remote stylesheets. (I found that pre patch, you could use the directory path in the Utils import within your remote sheet and it would be relative to root on the local system. However, after the patch, I had to also host the Utils file remotely and link import that one in the remote stylesheet)
4. It would be 'nice' to have some warning added to enable tracking through errors when a sheets fails to load. however, handling here is preferable to existing behaviour of falling back to an unhandled error in the browser.
5. I'm not sure I agree with how the sysprefs have been implemented, but this patch is consistent with current behaviour. [default - uses default stylesheets, path - is relative to root of filesystem, URL - uses remote stylesheets]

I'd prefer to see the path be relative to the root of the htdocs (and thereby allowing a simple Alias in the virtual host to point to the stylesheet.

Anywho, I don't see any reasons to fail.

*** Bug 12172 has been marked as a duplicate of this bug. ***

(In reply to Martin Renvoize from comment #15)
> Ok, also looks like the pref is still relative to root in the overall
> filetree and not the root htdocs location, and so this is not a regression.
> 
> This should be slightly better documented somewhere me thinks.

Thanks for testing!
I think we could adjust the description of XSLTDetailsDisplay and the others in the .pref files. But I would prefer to do that on a new report..

What we could do too, is: Add the possibility of a relative path to koha-tmpl by extending C4::XSLT's _get_best_default_xslt_filename.

(In reply to Martin Renvoize from comment #20)
> 3. I believe this enforces a more strict behaviour when using remote
> stylesheets. (I found that pre patch, you could use the directory path in
> the Utils import within your remote sheet and it would be relative to root
> on the local system. However, after the patch, I had to also host the Utils
> file remotely and link import that one in the remote stylesheet)

A relative path in the remote stylesheet is resolved within the remote current working directory. The old behavior of reading the text with GetURI and resolving the import path against the root of the local system was a side-effect that we should not want [to rely on].

> 4. It would be 'nice' to have some warning added to enable tracking through
> errors when a sheets fails to load. however, handling here is preferable to
> existing behaviour of falling back to an unhandled error in the browser.

I am attaching a QA follow-up that adds printing warns (via STDERR to the logfile) by default. Actually, that was former implicit behavior. You can now also disable that by saying $engine->print_warns(0).

Created attachment 28174 [details] [review]
Bug 11826: QA Follow-up: Enable printing warnings

Conform former (implicit) behavior the handler now prints warnings by
default (via STDERR to logfile).
This can be adjusted by: $engine->print_warns(0);

Test plan:
Generate some error on a XSLT file (e.g. wrong path).
Check if your log contains the associated error(s)/warning(s).

Pushed to master.  Thanks, Marcel!

I wonder if it would be useful to add a method for registering Perl functions with the XML::LibXSLT object.

I suppose we wouldn't want staff clients users to inject Perl code, so it would only be useful if we wanted to provide certain functions. I've used this before in the XSLTs to encode URIs and stuff like that. 

I suppose if we need to register functions, we can just do it on a case by case basis with whatever xslt handle is returned.

(In reply to David Cook from comment #26)
> I wonder if it would be useful to add a method for registering Perl
> functions with the XML::LibXSLT object.
> 
> I suppose we wouldn't want staff clients users to inject Perl code, so it
> would only be useful if we wanted to provide certain functions. I've used
> this before in the XSLTs to encode URIs and stuff like that. 
> 
> I suppose if we need to register functions, we can just do it on a case by
> case basis with whatever xslt handle is returned.

Interesting idea. I did not use these extensions functions, but I can imagine that some things are better done in Perl.
Why not open a new report Register functions in XSLT_Handler? You can do it :)

(In reply to M. de Rooy from comment #27)
> (In reply to David Cook from comment #26)
> > I wonder if it would be useful to add a method for registering Perl
> > functions with the XML::LibXSLT object.
> > 
> > I suppose we wouldn't want staff clients users to inject Perl code, so it
> > would only be useful if we wanted to provide certain functions. I've used
> > this before in the XSLTs to encode URIs and stuff like that. 
> > 
> > I suppose if we need to register functions, we can just do it on a case by
> > case basis with whatever xslt handle is returned.
> 
> Interesting idea. I did not use these extensions functions, but I can
> imagine that some things are better done in Perl.
> Why not open a new report Register functions in XSLT_Handler? You can do it
> :)

I might hold off for the moment, but if/when I get to pushing more code upstream, I might start with that first. I wouldn't want to add it as an enhancement without having a good use for it too.

(In reply to M. de Rooy from comment #27)
> (In reply to David Cook from comment #26)
> > I wonder if it would be useful to add a method for registering Perl
> > functions with the XML::LibXSLT object.
> > 
> > I suppose we wouldn't want staff clients users to inject Perl code, so it
> > would only be useful if we wanted to provide certain functions. I've used
> > this before in the XSLTs to encode URIs and stuff like that. 
> > 
> > I suppose if we need to register functions, we can just do it on a case by
> > case basis with whatever xslt handle is returned.
> 
> Interesting idea. I did not use these extensions functions, but I can
> imagine that some things are better done in Perl.
> Why not open a new report Register functions in XSLT_Handler? You can do it
> :)

After reading through the XML::LibXSLT documentation again (and some of the libxslt documentation), I've realized that it isn't necessary to register extension functions via the XML::LibXSLT object itself (and thus the XSLT_Handler object).

"XML::LibXSLT->register_function($uri, $name, $subref)" registers extension functions "globally", which can be done at the class level rather than the object level. That is, after you load the XML::LibXSLT class module, you can register functions regardless of the XSLTs that you need to parse or the objects you use to do so. 

If I understand correctly, it's because the XML::LibXSLT is just an interface to the libxslt C library. So you're not registering the extension function with an object but rather the actual XSLT engine itself. 

It might be worth noting that the documentation says this might not be thread or fork safe. I don't know what consequences that will have for folks running with Plack. 

I am guessing that every extension function you register will stick around until you restart Plack. I don't know if this will have any performance consequences. Extension functions are pretty common (they get used all the time in DSpace with the Xalan engine). So I don't know if there will be any negative consequences, but perhaps something to keep in mind.

I have however opened a bug to add parameters to the XSLT_Handler transform method.

This allows you to pass a hash of key => value pairs to use in a XSLT as parameters. I've used this in my OAI Harvesting code to pass in the unique record identifier from the OAI-PMH response to the XSLT so I can add it to the MARCXML that gets added to Koha.

Bug 13262 - Add parameters to XSLT Handler transform method
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13262

(In reply to David Cook from comment #30)
> I have however opened a bug to add parameters to the XSLT_Handler transform
> method.
> 
> This allows you to pass a hash of key => value pairs to use in a XSLT as
> parameters. I've used this in my OAI Harvesting code to pass in the unique
> record identifier from the OAI-PMH response to the XSLT so I can add it to
> the MARCXML that gets added to Koha.
> 
> Bug 13262 - Add parameters to XSLT Handler transform method
> http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13262

In SO now.