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.
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
Created attachment 43385 [details] MARC21 File for testing
Created attachment 43386 [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.
Created attachment 43412 [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>
> Signed-off-by: Mason James <mtj@kohaaloha.com> thanks Barton - patch works perfectly for me
Created attachment 43413 [details] [review] Bug 14716 - opac-detail.pl -- Series link fails when series - [FOLLOWUP] correct 2 other problematic series links
(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
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>
Created attachment 44175 [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>
Hm, wondering - why not remove the semicolon from the search term as we do in other places?
(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
(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
(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
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 :)
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>
(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'
(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
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.
*** Bug 13316 has been marked as a duplicate of this bug. ***
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.
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.
Removing the ; would be an incomplete fix, but much easier to do as a workaround solution.
(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)
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
Created attachment 72089 [details] [review] Bug 14716: URI-encode URLs in xsl templates.
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.
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!
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.
Neither the third patch alone nor all of them together apply. Can you please rebase?
Created attachment 73598 [details] [review] Bug 14716: URI-encode URLs in xsl templates. https://bugs.koha-community.org/show_bug.cgi?id=14176
Rebased. The last patch alone should apply.
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">
Paxed, yes, indeed. Thanks, I'll fix.
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.
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.
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?
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.
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.
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.
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.
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?
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.
Maybe too late yesterday, thx for the hint.
... 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.
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.
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.
Oops, now fixed.
Back on testing this - continueing later.
Signing off for MARC21 - maybe someone else could take a look for UNIMARC? Also tested that this fixes bug 16804. I prefer this solution.
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>
(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.
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>
QA Comment: Does this also need corresponding NORMARC changes?
(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.
Failing QA given the above comment.
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.
Magnus, can you please test the NORMARC patch?
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.
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
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>
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>
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.
Now rebased.
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.
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.
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.
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>
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>
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>
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>
Thanks for the swift followup. Looks great, works great, no regressions found. Passing QA
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/.
Awesome work all! Pushed to master for 18.11
Pushed to 18.05.x fo 18.05.06
I prefer not to push this change in 17.11.x
*** Bug 16804 has been marked as a duplicate of this bug. ***