Bug 14716 - Correctly URI-encode URLs in XSLT result lists and detail pages
Summary: Correctly URI-encode URLs in XSLT result lists and detail pages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Ere Maijala
QA Contact: Martin Renvoize
URL:
Keywords:
: 13316 16804 (view as bug list)
Depends on:
Blocks: 22085 24605 24654
  Show dependency treegraph
 
Reported: 2015-08-24 20:27 UTC by Barton Chittenden
Modified: 2020-02-13 15:49 UTC (History)
17 users (show)

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


Attachments
marc record with test data (3.09 KB, text/plain)
2015-08-24 20:27 UTC, Barton Chittenden
Details
MARC21 File for testing (12.06 KB, text/plain)
2015-10-13 15:33 UTC, Kyle M Hall
Details
Bug 14716 - opac-detail.pl -- Series link fails when series title ends with semicolon preceded by space. (5.83 KB, patch)
2015-10-13 15:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14716 - opac-detail.pl -- Series link fails when series title ends with semicolon preceded by space. (5.92 KB, patch)
2015-10-15 00:28 UTC, Mason James
Details | Diff | Splinter Review
Bug 14716 - opac-detail.pl -- Series link fails when series - [FOLLOWUP] (2.20 KB, patch)
2015-10-15 01:21 UTC, Mason James
Details | Diff | Splinter Review
Bug 14716 - opac-detail.pl -- Series link fails when series title ends with semicolon preceded by space. (5.87 KB, patch)
2015-10-29 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14716 - opac-detail.pl -- Series link fails when series - [FOLLOWUP] (2.20 KB, patch)
2015-10-29 17:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14716 - opac-detail.pl -- Series link fails when series - [FOLLOWUP] (2.30 KB, patch)
2015-12-17 00:38 UTC, Mason James
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in xsl templates. (31.38 KB, patch)
2018-02-22 14:05 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in xsl templates. (31.37 KB, patch)
2018-04-04 07:59 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in xsl templates. (60.17 KB, patch)
2018-04-04 10:53 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in xsl templates. (31.70 KB, patch)
2018-04-16 11:39 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in MARC21 xsl templates. (31.71 KB, patch)
2018-04-16 11:40 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in UNIMARC xsl templates. (4.54 KB, patch)
2018-04-16 11:41 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in MARC21 xsl templates. (32.28 KB, patch)
2018-04-24 13:05 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in MARC21 xsl templates. (32.33 KB, patch)
2018-06-25 22:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in UNIMARC xsl templates. (4.61 KB, patch)
2018-08-03 14:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in NORMARC xsl templates. (26.25 KB, patch)
2018-09-14 22:32 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in NORMARC xsl templates. (26.31 KB, patch)
2018-10-17 12:12 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in MARC21 xsl templates. (32.57 KB, patch)
2018-10-24 09:42 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in UNIMARC xsl templates. (4.60 KB, patch)
2018-10-24 09:42 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in NORMARC xsl templates. (26.31 KB, patch)
2018-10-24 09:42 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: Fix more unencoded url parameters. (52.82 KB, patch)
2018-10-26 15:21 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in MARC21 xsl templates. (32.63 KB, patch)
2018-10-29 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in UNIMARC xsl templates. (4.67 KB, patch)
2018-10-29 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14716: URI-encode URLs in NORMARC xsl templates. (26.37 KB, patch)
2018-10-29 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14716: Fix more unencoded url parameters. (52.88 KB, patch)
2018-10-29 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2015-08-24 20:27:22 UTC
Created attachment 41870 [details]
marc record with test data

To reproduce

Add a bib with the following data in biblioitems.marcxml:


  <datafield tag="440" ind1=" " ind2="0">
    <subfield code="a">Magic tree house ;</subfield>
    <subfield code="v">#6</subfield>
    <subfield code="9">39181</subfield>
  </datafield>


[the attached bib-14675.utf8 contains just such a record]

Make sure that the fast indexer picks up the record.

In the opac a title search for the bib; this should bring up opac-detail.pl

Using view source

The 'Series' link looks like this:

<a href="/cgi-bin/koha/opac-search.pl?q=se,phr:%22Magic%20tree%20house%20;%22">Magic tree house ; #6</a>

When url-decoded, the href looks like

/cgi-bin/koha/opac-search.pl?q=se,phr:"Magic tree house ;"

... this will not return any search results

When the trailing space and semicolon is removed:

/cgi-bin/koha/opac-search.pl?q=se,phr:"Magic tree house"

The resulting string can be url-encoded and appended to the opac base url -- this will return search results.
Comment 1 Kyle M Hall 2015-10-13 14:29:54 UTC
This is happening because the query is malformed.

Take for example the query se,phr:"Frank%20Einstein%20;" which is 

se,phr:"Frank Einstein"

but the semicolon is truncating the query so it looks to Koha 

se,phr:"Frank Einstein

which is not a problem on the staff side. Zebra seems to deal with this ok.

On the opac side though, we are adding on supression, so the query ends up like this:

(se,phr:"Frank Einstein ) not Suppress=1

which zebra chokes on.

Removing the semicolon from the subject heading would fix this, but I imagine that's a band-aid as the semi is part of the standard.

The semi is being treated as an ampersand. This is deep in the machinery ( http://www.w3.org/TR/1999/REC-html401-19991224/appendix/notes.html#h-B.2.2 )

Basically, the semi needs to be url encoded as %3B
Comment 2 Kyle M Hall 2015-10-13 15:33:45 UTC
Created attachment 43385 [details]
MARC21 File for testing
Comment 3 Kyle M Hall 2015-10-13 15:36:30 UTC Comment hidden (obsolete)
Comment 4 Mason James 2015-10-15 00:28:36 UTC Comment hidden (obsolete)
Comment 5 Mason James 2015-10-15 00:30:04 UTC
> Signed-off-by: Mason James <mtj@kohaaloha.com>

thanks Barton - patch works perfectly for me
Comment 6 Mason James 2015-10-15 01:21:19 UTC Comment hidden (obsolete)
Comment 7 Mason James 2015-10-15 01:23:24 UTC
(In reply to Mason James from comment #6)
> Created attachment 43413 [details] [review] [review]
> Bug 14716 - opac-detail.pl -- Series link fails when series - [FOLLOWUP]

switching back to 'needs signoff' for followup patch
Comment 8 Kyle M Hall 2015-10-29 17:14:57 UTC
Created attachment 44174 [details] [review]
Bug 14716 - opac-detail.pl -- Series link fails when series title ends with semicolon preceded by space.

This is happening because the query is malformed.

Take for example the query se,phr:"Frank%20Einstein%20;" which is

se,phr:"Frank Einstein"

but the semicolon is truncating the query so it looks to Koha

se,phr:"Frank Einstein

which is not a problem on the staff side. Zebra seems to deal with this
ok.

On the opac side though, we are adding on supression, so the query ends
up like this:

(se,phr:"Frank Einstein ) not Suppress=1

which zebra chokes on.

Removing the semicolon from the subject heading would fix this, but I
imagine that's a band-aid as the semi is part of the standard.

The semi is being treated as an ampersand. This is deep in the machinery
( http://www.w3.org/TR/1999/REC-html401-19991224/appendix/notes.html#h-B.2.2 )

Basically, the semi needs to be url encoded as %3B

Test Plan:
1) Import the records in the attached MARC21 file, re-index
2) From the OPAC search for "Frank Einstein and the antimatter motor"
3) Click the series link, note you get no results
4) Apply this patch
5) Refresh the record details page
6) Click the series link
7) Results!

NOTE: This bug does not occurr with all Koha server configurations.
      I have yet to determine the specific variable that creates
      this truncation behavior.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 9 Kyle M Hall 2015-10-29 17:15:07 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2015-11-05 07:12:52 UTC
Hm, wondering - why not remove the semicolon from the search term as we do in other places?
Comment 11 Mason James 2015-12-12 06:12:02 UTC
(In reply to Katrin Fischer from comment #10)
> Hm, wondering - why not remove the semicolon from the search term as we do
> in other places?

Hmmm... why remove characters from a search term, when you have a working solution that allows those characters to be used?

I think this solution is better
Comment 12 Mason James 2015-12-12 06:58:56 UTC
(In reply to Katrin Fischer from comment #10)
> Hm, wondering - why not remove the semicolon from the search term as we do
> in other places?

also, its not just the semicolon that is a problem character, 

its any character *other* than these...
 !'()*-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz~

so, we would have to remove all other characters from all search terms, using that method

its patch provides a general xslt solution to correctly encode problem characters within a url
Comment 13 Mason James 2015-12-12 07:18:00 UTC
(In reply to Mason James from comment #11)
> (In reply to Katrin Fischer from comment #10)
> > Hm, wondering - why not remove the semicolon from the search term as we do
> > in other places?
> 
> Hmmm... why remove characters from a search term, when you have a working
> solution that allows those characters to be used?
> 
> I think this solution is better

Cait, its not just the semicolon character that is causing a problem, here

afaict, its any char thats *not* the following...
  <!-- Characters that usually don't need to be escaped -->
!'()*-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz-

so, your solution would require us to remove all those problem chars from a 'search term', rather than using a technique that correctly url-encodes them all

If you are happy with my explaination, please change the status back to signed-off
Comment 14 Mason James 2015-12-12 07:21:09 UTC
oops, double post
(In reply to Mason James from comment #12)
> (In reply to Katrin Fischer from comment #10)
> > Hm, wondering - why not remove the semicolon from the search term as we do
> > in other places?
> 
> also, its not just the semicolon that is a problem character, 
> 

> Cait, its not just the semicolon character that is causing a problem, here
> 

oups, doubled posted :)
Comment 15 Mason James 2015-12-17 00:38:41 UTC
Created attachment 45743 [details] [review]
Bug 14716 - opac-detail.pl -- Series link fails when series - [FOLLOWUP]

correct 2 other problematic series links

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 16 Mason James 2015-12-17 00:42:38 UTC
(In reply to Mason James from comment #7)
> (In reply to Mason James from comment #6)
> > Created attachment 43413 [details] [review] [review] [review]
> > Bug 14716 - opac-detail.pl -- Series link fails when series - [FOLLOWUP]
> 
> switching back to 'needs signoff' for followup patch

follow-up patch is now 'signed-off'
Comment 17 Mason James 2015-12-17 00:51:50 UTC
(In reply to Mason James from comment #13)
> (In reply to Mason James from comment #11)
> > (In reply to Katrin Fischer from comment #10)
> > > Hm, wondering - why not remove the semicolon from the search term as we do
> > > in other places?
> 
> If you are happy with my explanation, please change the status back to
> signed-off

waiting for a response from Cait here
Comment 18 Katrin Fischer 2016-01-27 22:45:00 UTC
Hi, I am sorry, but I think the proposed solution doesn't work.

I tested using a 440 with characters outside latin1 - and the new function totally breaks the link. My example was 日本 (the word for Japan in Japanese) which is turned into ?? with the patch applied.
I'd be much happier with removing or encoding the known to be problematic characters in order to avoid problems with characters not in the list.

Another small thing: patches only touch the OPAC, but we should also include the intranet.
Comment 19 Owen Leonard 2016-06-21 18:27:08 UTC
*** Bug 13316 has been marked as a duplicate of this bug. ***
Comment 20 Eric Phetteplace 2016-08-11 22:23:46 UTC
If anyone else runs into this, I'm using a small bit of OPAC JavaScript as a work-around since many of records are affected:

// fix 830$a series links ending in semicolons, see bug #14716
$('.results_summary.series a').each(function(){
    // URI-encoded semicolon
    var href = $(this).attr('href').replace(/;/g, '%3B')
    $(this).attr('href', href)
})

Stuffing unencoded text inside URLs seems like a bad idea. If Japanese characters aren't working, maybe that's an issue with the url-encode function itself but not the approach taken to resolve this bug? I would think that almost anything being put in an href attribute would need to go through url-encode.
Comment 21 Katrin Fischer 2016-08-12 05:54:33 UTC
I am not against url-encoding, but we need to find a way to do this with xslt or outside the xslt that works better.
Comment 22 Katrin Fischer 2016-08-12 05:57:21 UTC
Removing the ; would be an incomplete fix, but much easier to do as a workaround solution.
Comment 23 Mason James 2017-05-09 02:59:33 UTC
(In reply to Katrin Fischer from comment #22)
> Removing the ; would be an incomplete fix, but much easier to do as a
> workaround solution.

..after re-readed this bug, i agree with Katrin :0)
Comment 24 Peggy Thrasher 2017-05-09 13:29:47 UTC
I would certainly support a patch that solves the semi-colon problem even if the other issues are not resolved.  If we could make the system work correctly when the MARC record follows the standard that would be a big improvement.

Thanks
Peggy
Comment 25 Ere Maijala 2018-02-22 14:05:48 UTC
Created attachment 72089 [details] [review]
Bug 14716: URI-encode URLs in xsl templates.
Comment 26 Ere Maijala 2018-02-22 14:12:24 UTC
I've attached a proposed fix for this. It uses the eslt str:encode-uri function to encode the href parameters. It's already in use with the link tracking in the OPAC templates. The patch contains changes for MARC21slim2intranetDetail.xsl, MARC21slim2intranetResults.xsl and MARC21slim2OPACDetail.xsl, but obviously this should be extended to any other templates. I just didn't want to spend the effort before getting feedback.

The only downside I can find with encode-uri is that the second parameter is required. If you find this too horrible, there's also the possibility of using register_function on the perl side to make a perl-based version available. It would allow even more control over the process.
Comment 27 Katrin Fischer 2018-03-30 11:29:29 UTC
Hi Ere, is your patch an alternative to the other 2 or a follow-up? Please make sure to always include a description and test plan in your commit messages!
Comment 28 Ere Maijala 2018-04-03 06:28:41 UTC
Katrin, my patch is alternative to the other ones and in my opinion the correct way to fix this and related encoding issues once and for all. But as I mentioned it's not complete yet and I'd like to have some feedback before I go through the trouble of completing it since it's a bit of tedious work. 

Kyle's version of url encoding would be fine otherwise, but it will only work for basic latin characters, and the current practice of shoving unencoded stuff into url's is simply wrong and only works because browsers are forgiving in the handling of most characters.
Comment 29 Katrin Fischer 2018-04-04 06:07:43 UTC
Neither the third patch alone nor all of them together apply. Can you please rebase?
Comment 30 Ere Maijala 2018-04-04 07:59:15 UTC
Created attachment 73598 [details] [review]
Bug 14716: URI-encode URLs in xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14176
Comment 31 Ere Maijala 2018-04-04 08:00:36 UTC
Rebased. The last patch alone should apply.
Comment 32 paxed 2018-04-04 09:27:36 UTC
Comment on attachment 73598 [details] [review]
Bug 14716: URI-encode URLs in xsl templates.

Ere, shouldn't this:

+            <xsl:when test="urlencode=1">

be

+            <xsl:when test="$urlencode=1">
Comment 33 Ere Maijala 2018-04-04 10:02:43 UTC
Paxed, yes, indeed. Thanks, I'll fix.
Comment 34 Ere Maijala 2018-04-04 10:53:13 UTC
Created attachment 73605 [details] [review]
Bug 14716: URI-encode URLs in xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14176

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.
Comment 35 Ere Maijala 2018-04-04 10:55:22 UTC
The bug Paxed noted has been fixed and the patch has been extended to cover all xsl files where I found search links (something with "q=") including UNIMARC and NORMARC. I did my best to test them too.
Comment 36 Katrin Fischer 2018-04-15 11:39:10 UTC
Hi Ere, 

you tend to create quite big patch sets - often it's better to split things up a bit to ease testing. That will make it easier for sign-offers.
For example you could have one patch for MARC21 and one for UNIMARC here. NORMARC might not be necessary, as of bug 18984.

From testing (MARC21):

1) URLs to external sources are broken at least in intranet result an detail page:
Example: http://localhost:8081/cgi-bin/koha/catalogue/http%3A%2F%2Fwww.loc.gov%2Fcatdir%2Fdescription%2Frandom048%2F90032101.html

2) Shoudl the comma behind publisher not be encoded?
http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=Provider:%22Vintage%20Books%2C%22
Record is from the sample data: Opus posthumous / by Wallace Stevens. 

Can you please check?
Comment 37 Ere Maijala 2018-04-16 11:39:10 UTC
Created attachment 74226 [details] [review]
Bug 14716: URI-encode URLs in xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.
Comment 38 Ere Maijala 2018-04-16 11:40:20 UTC
Created attachment 74227 [details] [review]
Bug 14716: URI-encode URLs in MARC21 xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.
Comment 39 Ere Maijala 2018-04-16 11:41:16 UTC
Created attachment 74228 [details] [review]
Bug 14716: URI-encode URLs in UNIMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
Comment 40 Ere Maijala 2018-04-16 11:44:43 UTC
Hi Katrin!

You're right, I should have split this. I've now split this into MARC21 and UNIMARC changes and omitted NORMARC. 

1. Thanks for catching the external url issue, it was definitely my bad. 

2. The comma is a reserved character and should be encoded. See e.g. https://en.wikipedia.org/wiki/Percent-encoding#Types_of_URI_characters for a list of reserved characters.
Comment 41 Katrin Fischer 2018-04-16 21:40:55 UTC
Hi Ere,

this is really strange: I am looking at http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=43 from the sample data. Which is a record with Hebew in 880. 

The place of publication and author name appear URL-encoded in Chromium, but not in Firefox. Any idea what could go on there?
Comment 42 Ere Maijala 2018-04-17 06:47:22 UTC
Katrin, Firefox tends to display things in clear even if they're sent properly encoded. You can use e.g. the network tab in the developer tools to see what's actually sent.
Comment 43 Katrin Fischer 2018-04-17 07:10:21 UTC
Maybe too late yesterday, thx for the hint.
Comment 44 Barton Chittenden 2018-04-17 15:05:24 UTC
... and Chrome will automagically URL encode at least *some* URLs (I'm reminded as I accidentally paste a '?' from an actual question into a bugzilla url, then have to delete '%3F' ). Again, the inspector is your friend.
Comment 45 Katrin Fischer 2018-04-23 05:26:28 UTC
Staff is looking good so far, but ran into an error on the OPAC side:
Error while transforming input: xmlXPathCompOpEval: function encode-uri bound to undefined prefix str
xmlXPathCompiledEval: 1 objects left on the stack.
runtime error: file /home/vagrant/kohaclone/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl line 273 element value-of
XPath evaluation returned no result.
 at /home/vagrant/kohaclone/Koha/XSLT_Handler.pm line 208.
Comment 46 Ere Maijala 2018-04-24 13:05:15 UTC
Created attachment 74804 [details] [review]
Bug 14716: URI-encode URLs in MARC21 xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.
Comment 47 Ere Maijala 2018-04-24 13:05:51 UTC
Oops, now fixed.
Comment 48 Katrin Fischer 2018-06-22 06:31:31 UTC
Back on testing this - continueing later.
Comment 49 Katrin Fischer 2018-06-25 21:59:08 UTC
Signing off for MARC21 - maybe someone else could take a look for UNIMARC?
Also tested that this fixes bug 16804. I prefer this solution.
Comment 50 Katrin Fischer 2018-06-25 22:01:54 UTC
Created attachment 76425 [details] [review]
Bug 14716: URI-encode URLs in MARC21 xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 51 Julian Maurice 2018-08-03 14:06:09 UTC
(In reply to Katrin Fischer from comment #49)
> Signing off for MARC21 - maybe someone else could take a look for UNIMARC?
> Also tested that this fixes bug 16804. I prefer this solution.

I confirm that the UNIMARC patch works too. I will attach the signed off patch.
Comment 52 Julian Maurice 2018-08-03 14:06:42 UTC
Created attachment 77471 [details] [review]
Bug 14716: URI-encode URLs in UNIMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 53 Martin Renvoize 2018-08-13 09:02:59 UTC
QA Comment: Does this also need corresponding NORMARC changes?
Comment 54 Magnus Enger 2018-08-13 09:21:11 UTC
(In reply to Martin Renvoize from comment #53)
> QA Comment: Does this also need corresponding NORMARC changes?

If this is fixing a problem in XSLT today it would be nice to fix it for NORMARC too. We will have to use NORMARC at least until the end of the year, I think, so fixing bugs is still welcome.
Comment 55 Martin Renvoize 2018-08-13 09:33:52 UTC
Failing QA given the above comment.
Comment 56 Ere Maijala 2018-09-14 22:32:47 UTC
Created attachment 78809 [details] [review]
Bug 14716: URI-encode URLs in NORMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
Comment 57 Katrin Fischer 2018-10-14 10:05:22 UTC
Magnus, can you please test the NORMARC patch?
Comment 58 Magnus Enger 2018-10-17 12:12:11 UTC
Created attachment 80701 [details] [review]
Bug 14716: URI-encode URLs in NORMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised.
Comment 59 Jonathan Druart 2018-10-23 15:19:36 UTC
First patch does not longer apply

Applying: Bug 14716: URI-encode URLs in MARC21 xsl templates.
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl
M       koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetResults.xsl
M       koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetResults.xsl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl
Comment 60 Ere Maijala 2018-10-24 09:42:11 UTC
Created attachment 81052 [details] [review]
Bug 14716: URI-encode URLs in MARC21 xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 61 Ere Maijala 2018-10-24 09:42:16 UTC
Created attachment 81053 [details] [review]
Bug 14716: URI-encode URLs in UNIMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 62 Ere Maijala 2018-10-24 09:42:20 UTC
Created attachment 81054 [details] [review]
Bug 14716: URI-encode URLs in NORMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised.
Comment 63 Ere Maijala 2018-10-24 09:42:43 UTC
Now rebased.
Comment 64 Martin Renvoize 2018-10-26 14:18:06 UTC
Great work.. as commented on irc earlier.. I found a few cases where we could be escaping but we're not yet:

1) Where we're searching the `an:` index. Unfortunately, we can't guarantee the marc field we're referring to won't contain any nasty characters.
2) rcn:<xsl:value-of select="marc:controlfield[@tag=001]"/> - 001 isn't entirely guaranteed to be clean either
3) The 'extractControlNumber' template appears to do it's own escaping.. perhaps we could convert this to use the str:encode-uri filter you're importing - This is by no means a failure case though.

That's all I can find.. nice work.
Comment 65 Ere Maijala 2018-10-26 15:21:21 UTC
Created attachment 81305 [details] [review]
Bug 14716: Fix more unencoded url parameters.

Fixes issues found during QA and unifies the SubfieldSelect function across the utils xsl's.
Comment 66 Ere Maijala 2018-10-26 15:24:28 UTC
I hope I managed to fix all the remaining issues. I left the removal of extra characters in the extractControlNumber template since just encoding them is not the same as getting rid of them, but I also added encoding.
Comment 67 Martin Renvoize 2018-10-29 15:30:55 UTC
Created attachment 81520 [details] [review]
Bug 14716: URI-encode URLs in MARC21 xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 68 Martin Renvoize 2018-10-29 15:31:00 UTC
Created attachment 81521 [details] [review]
Bug 14716: URI-encode URLs in UNIMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.
4. Also try the attached test records and make sure their links work.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 69 Martin Renvoize 2018-10-29 15:31:05 UTC
Created attachment 81522 [details] [review]
Bug 14716: URI-encode URLs in NORMARC xsl templates.

https://bugs.koha-community.org/show_bug.cgi?id=14716

Test plan:

1. Catalog a record with series name "Series & more"
2. Find the record
3. Make sure clicking the "Series & more" link works and the ampersand is properly encoded in the URL.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 70 Martin Renvoize 2018-10-29 15:31:10 UTC
Created attachment 81523 [details] [review]
Bug 14716: Fix more unencoded url parameters.

Fixes issues found during QA and unifies the SubfieldSelect function across the utils xsl's.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2018-10-29 15:31:51 UTC
Thanks for the swift followup.

Looks great, works great, no regressions found.

Passing QA
Comment 72 David Cook 2018-10-30 02:16:01 UTC
Cheers for working on this Ere!

I was going to suggest using XML::LibXSLT::register_function() to solve this problem, which I've done locally for years, but your comment reminded me of EXSLT! That seems like an easier way to do it. 

A while ago, I actually ended up using EXSLT in DSpace since Apache Xalan supports it and I needed some extension features. 

It's a very interesting read: http://exslt.org/.
Comment 73 Nick Clemens 2018-10-31 15:03:22 UTC
Awesome work all!

Pushed to master for 18.11
Comment 74 Martin Renvoize 2018-11-09 15:09:06 UTC
Pushed to 18.05.x fo 18.05.06
Comment 75 Fridolin Somers 2018-11-28 13:04:51 UTC
I prefer not to push this change in 17.11.x
Comment 76 Katrin Fischer 2020-01-11 14:16:55 UTC
*** Bug 16804 has been marked as a duplicate of this bug. ***