Bug 17807 - Use XSLT_Handler in oai.pl
Summary: Use XSLT_Handler in oai.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 19018
  Show dependency treegraph
 
Reported: 2016-12-21 15:22 UTC by Marcel de Rooy
Modified: 2018-06-04 20:10 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17807: Add format parameter to XSLT_Handler->transform (4.86 KB, patch)
2016-12-21 15:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17807: Use XSLT_Handler in Koha::OAI (4.78 KB, patch)
2016-12-21 15:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17807: Add format parameter to XSLT_Handler->transform (4.86 KB, patch)
2017-05-15 09:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17807: Use XSLT_Handler in Koha::OAI (4.91 KB, patch)
2017-05-15 09:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17807: Use XSLT_Handler in Koha::OAI (4.90 KB, patch)
2017-06-02 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17807: [Follow-up] Use marcxml for marc21 prefix (937 bytes, patch)
2017-06-06 11:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17807: Add format parameter to XSLT_Handler->transform (4.89 KB, patch)
2017-06-06 11:38 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17807: Use XSLT_Handler in Koha::OAI (4.93 KB, patch)
2017-06-06 11:38 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17807: [Follow-up] Use marcxml for marc21 prefix (967 bytes, patch)
2017-06-06 11:38 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17807: Add format parameter to XSLT_Handler->transform (4.93 KB, patch)
2017-08-07 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17807: Use XSLT_Handler in Koha::OAI (4.97 KB, patch)
2017-08-07 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17807: [Follow-up] Use marcxml for marc21 prefix (1010 bytes, patch)
2017-08-07 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17807: Add format parameter to XSLT_Handler->transform (5.02 KB, patch)
2017-08-30 19:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17807: Use XSLT_Handler in Koha::OAI (5.06 KB, patch)
2017-08-30 19:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17807: [Follow-up] Use marcxml for marc21 prefix (1.07 KB, patch)
2017-08-30 19:27 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2016-12-21 15:22:22 UTC

    
Comment 1 Marcel de Rooy 2016-12-21 15:33:27 UTC
Created attachment 58348 [details] [review]
Bug 17807: Add format parameter to XSLT_Handler->transform

Format may be chars (default), bytes or xmldoc.
Note: xmldoc is a XML::LibXML document object.
Since the default is chars, this does not affect current use.

Note: The format parameter (xmldoc) will be used later in one of the OAI
modules to prevent duplicated xml parsing.

Test plan:
Run t/db_dependent/XSLT_Handler.t
Comment 2 Marcel de Rooy 2016-12-21 15:52:30 UTC
Created attachment 58349 [details] [review]
Bug 17807: Use XSLT_Handler in Koha::OAI

Replacing some code in the OAI modules by a call to an existing module.
Note that the xmldoc format is used in the transform call to get a xml
document object.

The stylesheet method of Repository now only returns the name of the xsl
file to be used instead of a cached xslt object. Similar functionality
inside XSLT_Handler is used when calling transform.

Note: We still lack unit tests in this area. I did not see the need for
adding something for stylesheet since it only returns a simple string.
The other change is made in Record::new; there are no tests for this
module yet and the heart of the change here is actually tested already
in XSLT_Handler.t.

Note: I benchmarked calls to Repository in the old and the new situation
and did not see significant changes.

Test plan:
[1] Run t/db_dependent/Koha/OAI/Server.t
[2] Run oai.pl with ListRecords and marcxml.
[3] Run oai.pl with ListRecords and oai_dc.
Comment 3 Marcel de Rooy 2017-05-15 09:31:31 UTC
Created attachment 63463 [details] [review]
Bug 17807: Add format parameter to XSLT_Handler->transform

Format may be chars (default), bytes or xmldoc.
Note: xmldoc is a XML::LibXML document object.
Since the default is chars, this does not affect current use.

Note: The format parameter (xmldoc) will be used later in one of the OAI
modules to prevent duplicated xml parsing.

Test plan:
Run t/db_dependent/XSLT_Handler.t
Comment 4 Marcel de Rooy 2017-05-15 09:31:34 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-05-15 09:32:10 UTC
Rebased (waiting since Dec 21)
Comment 6 Marcel de Rooy 2017-06-02 09:54:59 UTC
Created attachment 63935 [details] [review]
Bug 17807: Use XSLT_Handler in Koha::OAI

Replacing some code in the OAI modules by a call to an existing module.
Note that the xmldoc format is used in the transform call to get a xml
document object.

The stylesheet method of Repository now only returns the name of the xsl
file to be used instead of a cached xslt object. Similar functionality
inside XSLT_Handler is used when calling transform.

Note: We still lack unit tests in this area. I did not see the need for
adding something for stylesheet since it only returns a simple string.
The other change is made in Record::new; there are no tests for this
module yet and the heart of the change here is actually tested already
in XSLT_Handler.t.

Note: I benchmarked calls to Repository in the old and the new situation
and did not see significant changes.

Test plan:
[1] Run t/db_dependent/OAI/Server.t
[2] Run oai.pl with ListRecords and marcxml.
[3] Run oai.pl with ListRecords and oai_dc.
Comment 7 Josef Moravec 2017-06-02 19:31:30 UTC
Marcel, when I try to get record in marc21 metadata format, I am getting the oai_dc record.... I should get marcxml....
Comment 8 Marcel de Rooy 2017-06-06 10:40:59 UTC
(In reply to Josef Moravec from comment #7)
> Marcel, when I try to get record in marc21 metadata format, I am getting the
> oai_dc record.... I should get marcxml....

I tested again and I see DC output and marcxml.
Did you restart Plack?
Can you show the URLs you are using?
Comment 9 Josef Moravec 2017-06-06 10:59:08 UTC
/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=oai_dc
--> oai_dc, that's OK

/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
--> marcxml, that's OK

/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marc21
--> oai_dc, that's NOT OK
Comment 10 Marcel de Rooy 2017-06-06 11:34:52 UTC
Created attachment 64045 [details] [review]
Bug 17807: [Follow-up] Use marcxml for marc21 prefix

Not sure what happened here, but this is an easy fix.

Test plan:
Try oai.pl with marcxml, marc21 and oai_dc again.
Comment 11 Josef Moravec 2017-06-06 11:38:29 UTC
Created attachment 64046 [details] [review]
[SIGNED-OFF] Bug 17807: Add format parameter to XSLT_Handler->transform

Format may be chars (default), bytes or xmldoc.
Note: xmldoc is a XML::LibXML document object.
Since the default is chars, this does not affect current use.

Note: The format parameter (xmldoc) will be used later in one of the OAI
modules to prevent duplicated xml parsing.

Test plan:
Run t/db_dependent/XSLT_Handler.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2017-06-06 11:38:32 UTC
Created attachment 64047 [details] [review]
[SIGNED-OFF] Bug 17807: Use XSLT_Handler in Koha::OAI

Replacing some code in the OAI modules by a call to an existing module.
Note that the xmldoc format is used in the transform call to get a xml
document object.

The stylesheet method of Repository now only returns the name of the xsl
file to be used instead of a cached xslt object. Similar functionality
inside XSLT_Handler is used when calling transform.

Note: We still lack unit tests in this area. I did not see the need for
adding something for stylesheet since it only returns a simple string.
The other change is made in Record::new; there are no tests for this
module yet and the heart of the change here is actually tested already
in XSLT_Handler.t.

Note: I benchmarked calls to Repository in the old and the new situation
and did not see significant changes.

Test plan:
[1] Run t/db_dependent/OAI/Server.t
[2] Run oai.pl with ListRecords and marcxml.
[3] Run oai.pl with ListRecords and oai_dc.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2017-06-06 11:38:35 UTC
Created attachment 64048 [details] [review]
[SIGNED-OFF] Bug 17807: [Follow-up] Use marcxml for marc21 prefix

Not sure what happened here, but this is an easy fix.

Test plan:
Try oai.pl with marcxml, marc21 and oai_dc again.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2017-06-06 11:39:32 UTC
(In reply to Marcel de Rooy from comment #10)
> Created attachment 64045 [details] [review] [review]
> Bug 17807: [Follow-up] Use marcxml for marc21 prefix
> 
> Not sure what happened here, but this is an easy fix.
> 
> Test plan:
> Try oai.pl with marcxml, marc21 and oai_dc again.

Thanks for quick followup, now it's working perfect, so I signing off ;)
Comment 15 Marcel de Rooy 2017-06-06 11:43:16 UTC
(In reply to Josef Moravec from comment #14)

> Thanks for quick followup, now it's working perfect, so I signing off ;)

Same for you !
Comment 16 Nick Clemens 2017-07-31 13:25:49 UTC
Everything seems to work fine here, however, errors are really noisy if the file is not found - this shouldn't be an issue really since file isn't configurable, but should it be a little quieter? Waiting for answer before PQA
Comment 17 Marcel de Rooy 2017-08-01 13:41:19 UTC
(In reply to Nick Clemens from comment #16)
> Everything seems to work fine here, however, errors are really noisy if the
> file is not found - this shouldn't be an issue really since file isn't
> configurable, but should it be a little quieter? Waiting for answer before
> PQA

File should be there obviously, but you are right. We could reduce the noise a bit. Opened a new report for that.
Comment 18 Nick Clemens 2017-08-07 12:27:59 UTC
Created attachment 65534 [details] [review]
Bug 17807: Add format parameter to XSLT_Handler->transform

Format may be chars (default), bytes or xmldoc.
Note: xmldoc is a XML::LibXML document object.
Since the default is chars, this does not affect current use.

Note: The format parameter (xmldoc) will be used later in one of the OAI
modules to prevent duplicated xml parsing.

Test plan:
Run t/db_dependent/XSLT_Handler.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens 2017-08-07 12:28:05 UTC
Created attachment 65535 [details] [review]
Bug 17807: Use XSLT_Handler in Koha::OAI

Replacing some code in the OAI modules by a call to an existing module.
Note that the xmldoc format is used in the transform call to get a xml
document object.

The stylesheet method of Repository now only returns the name of the xsl
file to be used instead of a cached xslt object. Similar functionality
inside XSLT_Handler is used when calling transform.

Note: We still lack unit tests in this area. I did not see the need for
adding something for stylesheet since it only returns a simple string.
The other change is made in Record::new; there are no tests for this
module yet and the heart of the change here is actually tested already
in XSLT_Handler.t.

Note: I benchmarked calls to Repository in the old and the new situation
and did not see significant changes.

Test plan:
[1] Run t/db_dependent/OAI/Server.t
[2] Run oai.pl with ListRecords and marcxml.
[3] Run oai.pl with ListRecords and oai_dc.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2017-08-07 12:28:10 UTC
Created attachment 65536 [details] [review]
Bug 17807: [Follow-up] Use marcxml for marc21 prefix

Not sure what happened here, but this is an easy fix.

Test plan:
Try oai.pl with marcxml, marc21 and oai_dc again.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Jonathan Druart 2017-08-30 19:27:00 UTC
Created attachment 66635 [details] [review]
Bug 17807: Add format parameter to XSLT_Handler->transform

Format may be chars (default), bytes or xmldoc.
Note: xmldoc is a XML::LibXML document object.
Since the default is chars, this does not affect current use.

Note: The format parameter (xmldoc) will be used later in one of the OAI
modules to prevent duplicated xml parsing.

Test plan:
Run t/db_dependent/XSLT_Handler.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2017-08-30 19:27:04 UTC
Created attachment 66636 [details] [review]
Bug 17807: Use XSLT_Handler in Koha::OAI

Replacing some code in the OAI modules by a call to an existing module.
Note that the xmldoc format is used in the transform call to get a xml
document object.

The stylesheet method of Repository now only returns the name of the xsl
file to be used instead of a cached xslt object. Similar functionality
inside XSLT_Handler is used when calling transform.

Note: We still lack unit tests in this area. I did not see the need for
adding something for stylesheet since it only returns a simple string.
The other change is made in Record::new; there are no tests for this
module yet and the heart of the change here is actually tested already
in XSLT_Handler.t.

Note: I benchmarked calls to Repository in the old and the new situation
and did not see significant changes.

Test plan:
[1] Run t/db_dependent/OAI/Server.t
[2] Run oai.pl with ListRecords and marcxml.
[3] Run oai.pl with ListRecords and oai_dc.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2017-08-30 19:27:08 UTC
Created attachment 66637 [details] [review]
Bug 17807: [Follow-up] Use marcxml for marc21 prefix

Not sure what happened here, but this is an easy fix.

Test plan:
Try oai.pl with marcxml, marc21 and oai_dc again.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2017-08-30 19:57:09 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 25 Fridolin Somers 2017-09-29 13:37:12 UTC
Enhancement not pushed to 17.05.x