Bug 13262 - Add parameters to XSLT Handler transform method
Summary: Add parameters to XSLT Handler transform method
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-17 02:55 UTC by David Cook
Modified: 2015-12-03 22:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13262 - Add parameters to XSLT Handler transform method (4.80 KB, patch)
2014-11-17 04:42 UTC, David Cook
Details | Diff | Splinter Review
Bug 13262 - Add parameters to XSLT Handler transform method (4.89 KB, patch)
2014-11-17 07:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13262 - Add parameters to XSLT Handler transform method (4.81 KB, patch)
2014-11-26 23:19 UTC, David Cook
Details | Diff | Splinter Review
Bug 13262 - Add parameters to XSLT Handler transform method (4.91 KB, patch)
2014-11-27 07:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 13262 - Add parameters to XSLT Handler transform method (4.93 KB, patch)
2015-01-02 18:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13262 - Add parameters to XSLT Handler transform method (5.69 KB, patch)
2015-01-27 23:40 UTC, David Cook
Details | Diff | Splinter Review
Bug 13262: (QA followup) fix duplicate variable definition (1.11 KB, patch)
2015-02-05 18:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2014-11-17 02:55:43 UTC
The XML::LibXSLT transform method takes a XSLT and an optional hash of key => value pairs to use in the XSLT as parameters. 

This enhancement would allow you to pass parameters to the XSLT when using the Koha::XSLT_Handler::transform method as well.
Comment 1 David Cook 2014-11-17 04:42:16 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2014-11-17 07:56:02 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-11-21 14:54:04 UTC
Got a very odd UT failure:
not ok 26 - Successfully injected string into XSLT parameter/variable

#   Failed test 'Successfully injected string into XSLT parameter/variable'
#   at t/db_dependent/XSLT_Handler.t line 128.
# An error occurred while using eq:
# ------------------------------------
# Operation "eq": no method found,
# 	left argument in overloaded package XML::LibXML::NodeList,
# 	right argument has no overloaded magic at (eval in cmp_ok) t/db_dependent/XSLT_Handler.t line 128.
#
# ------------------------------------
#          got: 'this is a test'
#     expected: 'this is a test'


Full UT Output:

prove -v t/db_dependent/XSLT_Handler.t
t/db_dependent/XSLT_Handler.t ..
1..26
ok 1 - Testing creation of handler object
ok 2 - Engine returns error on no file
ok 3 - Engine returns error on bad file
ok 4 - Test on invalid refresh
ok 5 - Found my test stylesheet test01.xsl
ok 6 - Engine returns error on undefined text
No XSLT file passed. at Koha/XSLT_Handler.pm line 322.
XSLT file not found. at Koha/XSLT_Handler.pm line 322.
No string to transform. at Koha/XSLT_Handler.pm line 322.
Error while parsing input: Empty String at Koha/XSLT_Handler.pm line 179.
ok 7 - Engine returns error on empty string
Error while parsing input: :1: parser error : Start tag expected, '<' not found
abcdef
^
ok 8 - Engine returns error on non-xml
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 9 - Engine returns error on malformed xml
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 10 - Engine respects do_not_return_source==1
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 11 - Second engine respects it too
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 12 - Engine respects do_not_return_source==0
ok 13 - Test on valid refresh
ok 14 - Test on repeated refresh
ok 15 - Engine returned no error for xml_1
ok 16 - No real change expected for xml_1
ok 17 - Engine returned no error for xml_2
ok 18 - Saw the expected change for xml_2
ok 19 - Found my test stylesheet test02.xsl
ok 20 - Engine returned error for parsing bad xsl
ok 21 - Error string contains text
ok 22 - Found my test stylesheet test03.xsl
ok 23 - Unexpected error on transform with third xsl
ok 24 - Final test on clearing cache
ok 25 - Found my test stylesheet test04.xsl
not ok 26 - Successfully injected string into XSLT parameter/variable
Error while parsing stylesheet: compilation error: file /var/lib/koha/kohaqa/kohaclone/t/db_dependent/XSLT_Handler/test02.xsl element variable
redefinition of global variable redefine
 at Koha/XSLT_Handler.pm line 274.

#   Failed test 'Successfully injected string into XSLT parameter/variable'
#   at t/db_dependent/XSLT_Handler.t line 128.
# An error occurred while using eq:
# ------------------------------------
# Operation "eq": no method found,
# 	left argument in overloaded package XML::LibXML::NodeList,
# 	right argument has no overloaded magic at (eval in cmp_ok) t/db_dependent/XSLT_Handler.t line 128.
#
# ------------------------------------
#          got: 'this is a test'
#     expected: 'this is a test'
# Looks like you failed 1 test of 26.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/26 subtests

Test Summary Report
-------------------
t/db_dependent/XSLT_Handler.t (Wstat: 256 Tests: 26 Failed: 1)
  Failed test:  26
  Non-zero exit status: 1
Files=1, Tests=26,  0 wallclock secs ( 0.02 usr  0.01 sys +  0.05 cusr  0.00 csys =  0.08 CPU)
Result: FAIL
Comment 4 Marcel de Rooy 2014-11-23 12:47:22 UTC
All tests successful.
Files=1, Tests=26,  0 wallclock secs ( 0.03 usr  0.00 sys +  0.07 cusr  0.00 csys =  0.10 CPU)
Result: PASS
Comment 5 Marcel de Rooy 2014-11-23 13:06:21 UTC
Maybe we should just be pragmatic here and replace the lines:

require XML::LibXML;
my $dom = XML::LibXML->load_xml(string => $output);
my $result = $dom->find( '/just_a_tagname' );

by a simple test like index($output,'this is a test')>0

LibXML uses overloading in XML::LibXML::Literal (and others) called from XML::LibXML::Node. Debugging this stuff seems much harder than just simplifying the test..
Comment 6 David Cook 2014-11-24 23:27:26 UTC
(In reply to M. de Rooy from comment #5)
> Maybe we should just be pragmatic here and replace the lines:
> 
> require XML::LibXML;
> my $dom = XML::LibXML->load_xml(string => $output);
> my $result = $dom->find( '/just_a_tagname' );
> 
> by a simple test like index($output,'this is a test')>0
> 
> LibXML uses overloading in XML::LibXML::Literal (and others) called from
> XML::LibXML::Node. Debugging this stuff seems much harder than just
> simplifying the test..

Maybe. Using index(), while not as exact, might be "good enough".

However, I'm rather intrigued that Kyle is getting errors while you (Marcel) and I are not. I have no idea why it warned "left argument in overloaded package XML::LibXML::NodeList" when it should've been a XML::LibXML::Literal which would've magically stringified.

Maybe rather than using index(), we should look into why Kyle is getting that problem though. The answer will determine the solution.

If it's an issue of the overloading not working as expected, it would probably be an idea to go with:

ok($result->value() eq 'this is a test', 'test name');

However, that still won't work if Kyle really is getting a NodeList back instead of a Literal.

Kyle: Could you do a Data::Dumper($result) and tell us what you get?
Comment 7 David Cook 2014-11-26 23:19:57 UTC Comment hidden (obsolete)
Comment 8 David Cook 2014-11-26 23:23:45 UTC
I don't know why the original test failed for Kyle when it worked for Marcel and myself.

However, I've explicitly added a to_literal() method to the result, which should make the test work for everyone. I imagine it was due to having different versions of XML::LibXML.

I was wrong about it being a XML::LibXML::Literal though. It was actually a XML::LibXML::Element within a XML::LibXML::NodeList, which makes sense as the XPATH results an element. I could've been more specific and had it grab the element's child text node, but it doesn't really matter. Doing "to_literal()" will be perfectly fine. Better even as it makes sure nothing else gets into that element than what we put into it.
Comment 9 Marcel de Rooy 2014-11-27 07:33:18 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2014-11-27 07:34:14 UTC
Would be interesting if it also works for Kyle now :)
Comment 11 David Cook 2014-11-28 01:15:00 UTC
(In reply to M. de Rooy from comment #10)
> Would be interesting if it also works for Kyle now :)

Agreed. I'm guessing yes, but I'm eager to find out :)
Comment 12 Kyle M Hall 2015-01-02 18:51:28 UTC
prove -v t/db_dependent/XSLT_Handler.t
t/db_dependent/XSLT_Handler.t ..
1..26
ok 1 - Testing creation of handler object
No XSLT file passed. at Koha/XSLT_Handler.pm line 322.
ok 2 - Engine returns error on no file
XSLT file not found. at Koha/XSLT_Handler.pm line 322.
ok 3 - Engine returns error on bad file
ok 4 - Test on invalid refresh
ok 5 - Found my test stylesheet test01.xsl
No string to transform. at Koha/XSLT_Handler.pm line 322.
ok 6 - Engine returns error on undefined text
Error while parsing input: Empty String at Koha/XSLT_Handler.pm line 179.
ok 7 - Engine returns error on empty string
Error while parsing input: :1: parser error : Start tag expected, '<' not found
abcdef
^
ok 8 - Engine returns error on non-xml
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 9 - Engine returns error on malformed xml
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 10 - Engine respects do_not_return_source==1
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 11 - Second engine respects it too
Error while parsing input: :1: parser error : Opening and ending tag mismatch: a line 1 and b
<a></b>
       ^
ok 12 - Engine respects do_not_return_source==0
ok 13 - Test on valid refresh
ok 14 - Test on repeated refresh
ok 15 - Engine returned no error for xml_1
ok 16 - No real change expected for xml_1
ok 17 - Engine returned no error for xml_2
ok 18 - Saw the expected change for xml_2
ok 19 - Found my test stylesheet test02.xsl
Error while parsing stylesheet: compilation error: file /var/lib/koha/kohaqa/kohaclone/t/db_dependent/XSLT_Handler/test02.xsl element variable
redefinition of global variable redefine
 at Koha/XSLT_Handler.pm line 274.
ok 20 - Engine returned error for parsing bad xsl
ok 21 - Error string contains text
ok 22 - Found my test stylesheet test03.xsl
ok 23 - Unexpected error on transform with third xsl
ok 24 - Final test on clearing cache
ok 25 - Found my test stylesheet test04.xsl
ok 26 - Successfully injected string into XSLT parameter/variable
ok
All tests successful.
Files=1, Tests=26,  0 wallclock secs ( 0.03 usr  0.02 sys +  0.10 cusr  0.01 csys =  0.16 CPU)
Result: PASS
Comment 13 Kyle M Hall 2015-01-02 18:52:06 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-01-02 18:52:56 UTC
I assume the errors during the unit tests are not a problem since the test data is purposely malformed.
Comment 15 Tomás Cohen Arazi 2015-01-26 19:19:46 UTC
Things changed since this one got passed, please rebase so I can push this nice enhancement.
Comment 16 David Cook 2015-01-27 23:40:30 UTC
Created attachment 35566 [details] [review]
Bug 13262 - Add parameters to XSLT Handler transform method

This patch adds an optional hashref argument to the XSLT_Handler
transform() method. It allows you to send key => value pairs
parameters to the XML::LibXSLT object, which you can reference
in a XSLT via <xsl:param name="XXX" />.

The parameter value is evaluated as an XPath query, so you can only
pass quoted strings (i.e. "'test'") or numbers. Otherwise, the
XSLT engine will interpret it as a Xpath query and will run it
on the XML that you're transforming.

The most common use case is sending strings to a XSLT. In my case,
this is an OAI-PMH identifier that comes in a OAI response but not
the actual metadata. See the following link from the official POD:
http://search.cpan.org/~shlomif/XML-LibXSLT-1.92/LibXSLT.pm#Parameters

_TEST PLAN_

1) Run "perl t/db_dependent/XSLT_Handler.t". If all tests pass,
you should be free to sign off. Feel free to inspect the last
test in XSLT_Handler.t and the XSL in test04.xsl to see how it
works.

If you really want to be thorough, you could write your own test
cases using mine as an example.

Alternatively, you could go into C4::XSLT, and try to pass a
value to a parameter in the search results or the detail page,
but that might be a bit over the top.

It's a pretty simple patch.
Comment 17 David Cook 2015-01-27 23:44:01 UTC
Comment on attachment 34915 [details] [review]
[PASSED QA] Bug 13262 - Add parameters to XSLT Handler transform method

Should've probably applied and modified this one... but just rebased original patch...
Comment 18 Tomás Cohen Arazi 2015-02-05 18:28:07 UTC
Created attachment 35685 [details] [review]
Bug 13262: (QA followup) fix duplicate variable definition

Running

 $ prove t/db_dependent/XSLT_Handler.t

raises a couple warnings because of variable redefinitions. This small patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 19 Tomás Cohen Arazi 2015-02-05 18:31:03 UTC
Patch pushed to master.

Thanks David! It opens us for interesting improvements.
Comment 20 David Cook 2015-02-05 22:23:02 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> Patch pushed to master.
> 
> Thanks David! It opens us for interesting improvements.

Thanks, Tomas! I hope so! 

I think it would be interested to try XML::LibXSLT->register_function($uri, $name, $subref) as well. I use this locally for a few XSLT mods, and it works rather nicely. It doesn't require any mods to the Koha::XSLT_Handler either. You just "use XML::LibXSLT", register a function, and you're good to go!