and not html filtered, see the wiki page for more details https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML9:_filter_all_the_variables
Created attachment 80337 [details] [review] Bug 21526: uri escape TT variables used to build a link
Created attachment 80641 [details] [review] Bug 21526: uri escape TT variables when used in 'a href' This patch has been generated with the script provided on bug 21576. It only affects variable used in the href attribute of a link *when* href it the first attribute of the node (grep "a href")
Created attachment 80649 [details] [review] Bug 21526: uri escape TT variables when used in 'a href' This patch has been generated with the script provided on bug 21576. It only affects variable used in the href attribute of a link *when* href it the first attribute of the node (grep "a href")
Created attachment 80661 [details] [review] Bug 21526: Fix html filters (new pushes)
Comment on attachment 80649 [details] [review] Bug 21526: uri escape TT variables when used in 'a href' Review of attachment 80649 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/includes/patron-title.inc @@ +1,1 @@ > +[% USE raw %] Hmm.. I wasn't aware of the differing syntax available here.. where's the difference between [% USE raw %] and [%- USE raw -%] if any?
(In reply to Martin Renvoize from comment #5) > Hmm.. I wasn't aware of the differing syntax available here.. where's the > difference between [% USE raw %] and [%- USE raw -%] if any? It changes how whitespaces are chomped. See http://template-toolkit.org/docs/manual/Syntax.html#section_Chomping_Whitespace and http://template-toolkit.org/docs/manual/Config.html#section_PRE_CHOMP_POST_CHOMP
And so you found a bug, congrats :) +[% USE raw %] [%- USE raw -%] => This is wrong, the test to know if the USE statement exist does not the chomp chars (- ~) into account
Created attachment 80725 [details] [review] Bug 21526: Remove duplicated USE raw statement See patch on bug 21576 comment 14
Created attachment 80755 [details] [review] Bug 21526: uri escape TT variables when used in 'a href' This patch has been generated with the script provided on bug 21576. It only affects variable used in the href attribute of a link *when* href it the first attribute of the node (grep "a href")
Created attachment 80756 [details] [review] Bug 21526: Remove duplicated USE raw statement See patch on bug 21576 comment 14
Created attachment 81058 [details] [review] Bug 21526: uri escape TT variables when used in 'a href' This patch has been generated with the script provided on bug 21576. It only affects variable used in the href attribute of a link *when* href it the first attribute of the node (grep "a href") Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81059 [details] [review] Bug 21526: Remove duplicated USE raw statement See patch on bug 21576 comment 14 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Looking good, can't find any regressions in testing :)
Created attachment 81300 [details] [review] [PUSHED] Bug 21526: uri escape TT variables when used in 'a href' This patch has been generated with the script provided on bug 21576. It only affects variable used in the href attribute of a link *when* href it the first attribute of the node (grep "a href") Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81301 [details] [review] [PUSHED] Bug 21526: Remove duplicated USE raw statement See patch on bug 21576 comment 14 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 81302 [details] [review] [PUSHED] Bug 21526: (QA follow-up) Fix all the tabs
Awesome work all! Pushed to master for 18.11
Created attachment 81337 [details] [review] [PUSHED] Bug 21526: (RM follow-up) Fix merge markers and a few extra filters Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 81530 [details] [review] Bug 21526: Use the 'url' filter when needed
(In reply to Jonathan Druart from comment #19) > Created attachment 81530 [details] [review] [review] > Bug 21526: Use the 'url' filter when needed This will break/not fix the facets and other links from the result page.
Comment on attachment 81530 [details] [review] Bug 21526: Use the 'url' filter when needed Review of attachment 81530 [details] [review]: ----------------------------------------------------------------- Generally, if there was no comment, I don't think there was a problem, OR... I commented about a problem elsewhere, and didn't feel like repeating it over and over. Here's some feedback. There ARE some Failed QA issues. ::: koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search-results.inc @@ +10,4 @@ > > [% IF marcflavour == 'UNIMARC' %] > [% IF authid %] > + [% link = BLOCK %]<a href="[% authidurl _ authid | url %]">[% heading | html %]</a>[% END %] Perhaps this line and line 31 should use the same method to build the URL? I tend to prefer 31's split. @@ +28,4 @@ > <span class="heading"> > [% IF ( linkType=='seealso' ) %] > [% IF ( authid ) %] > + <a href="[% authidurl | url %][% authid | uri %]">[% heading | html %]</a> Nice catch on the URL vs URI. But I think URI should be used for the base part, and URL for anything we are adding to it, unless there is a bizarre & at the end or something. ::: koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc @@ +33,4 @@ > [% IF facet.active %] > [% SET local_url = url _ "&nolimit=" _ facet.type_link_value _ ":" _ facet.facet_link_value %] > <span class="facet-label">[% facet.facet_label_value | html %]</span> > + [<a href="[% local_url | url %]" title="Remove facet [% facet.facet_link_value | html %]">x</a>] Yes. Because local_url is the WHOLE url, the url filter is right. @@ +45,4 @@ > [% END %] > [% IF ( facets_loo.expandable ) %] > <li class="showmore"> > + <a href="[% url | url %][% IF offset %]&offset=[% offset |url %][% END %]&expand=[% facets_loo.expand |url %]#[% facets_loo.type_id |url %]">Show more</a> If url is more than the base url, then url is correct, but the others are single values, so I would think it is uri. ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt @@ +413,4 @@ > </td> > <td><span title="[% batch_lis.staged_date | html %]">[% batch_lis.staged_date | $KohaDates with_hours => 1 | html %]</span></td> > <td>[% batch_lis.num_records | html %]</td> > + <td><a href="[% batch_lis.scriptname | url %]?import_batch_id=[% batch_lis.import_batch_id | uri %]&basketno=[% basketno | uri %]&booksellerid=[% booksellerid | uri %]">Add orders</a></td> Nice correction of the uri to url. Also, see the use of uri here for the token-y parts? That's what I would expect. ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt @@ +2,4 @@ > [% USE Asset %] > [% BLOCK csv_export %] > <div class="btn-group"> > + <a id="exportbutton" class="btn btn-default btn-sm" href="[% script_name | url %]?op=export&basketno=[% basketno | html %]&booksellerid=[% booksellerid | html %]"><i class="fa fa-download"></i> Export as CSV</a> Okay, so should the token-y parts be uri or html? Hmmm... html, to prevent security attacks? Perhaps all the tiny parts should be html. So far, it hasn't been consisten. ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/supplier.tt @@ +264,4 @@ > <p><span class="label">Phone: </span>[% phone | html %]</p> > <p><span class="label">Fax: </span>[% fax | html %]</p> > [% IF ( url ) %] > + <p><span class="label">Website: </span><a href="[% url | url %]">[% url | html %]</a></p> Right, the url filter for a full URL, and the html filter to display it without running any sort of weird scripting security side effects. ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt @@ +96,4 @@ > </ol></fieldset> > <fieldset class="action"> > <input type="submit" value="Save changes" /> > + <a class="cancel" href="[% script_name | url %]?frameworkcode=[% framework.frameworkcode | html %]">Cancel</a> missing changing an html to uri. ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/printslip.tt @@ +13,4 @@ > [% END %] > [% INCLUDE 'doc-head-close.inc' %] > <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > +<link rel="shortcut icon" href="[% IF ( IntranetFavicon ) %][% IntranetFavicon | %][% ELSE %][% interface | html %]/[% theme | html %]/img/favicon.ico[% END %]" type="image/x-icon" /> BAD FILTER! IntranetFavicon missing a filter name. If interface is a url, then html is the wrong filter. theme probably should be uri. ::: koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt @@ +405,4 @@ > <input type="text" readonly="readonly" id="total" name="total" size="10" value="[% total | html %]"/> > </li></ol> > </fieldset><input type="hidden" id="returnsuggested" name="returnsuggested" value="[% IF ( returnsuggestedby ) %][% returnsuggestedby | html %][% ELSE %]noone[% END %]"/> > + <fieldset class="action"><input type="hidden" name="op" value="[% op | html %]" />[% IF ( suggestionid ) %]<input type="submit" value="Save" /> <a class="cancel" href="[% IF ( returnsuggestedby ) %]/cgi-bin/koha/members/moremember.pl?borrowernumber=[% returnsuggestedby | uri %]#suggestions[% ELSE %]suggestion.pl[% END %]">Cancel</a>[% ELSE %]<input type="submit" value="Submit your suggestion" /> <a class="cancel" href="suggestion.pl">Cancel</a>[% END %] Nice correction of html to uri in the url build part, but the hidden value's value... perhaps uri? After all, it's like the portion of a long GET request, not a string to display. ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-basket.tt @@ +201,4 @@ > [% IF MARCurl.part %]<p>[% MARCurl.part | html %]</p>[% END %] > > [% IF OPACURLOpenInNewWindow %] > + <a href="[% MARCurl.MARCURL | url %]" title="[% MARCurl.MARCURL | html %]" target="_blank" rel="noreferrer">[% MARCurl.linktext | html %]</a> Should note that I agree that title should use html filter, because it's just a tooltip, not actually a link value. ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt @@ +337,4 @@ > > [% IF ( OPACURLOpenInNewWindow ) %] > [% IF trackclicks == 'track' || trackclicks == 'anonymous' %] > + <a href="/cgi-bin/koha/tracklinks.pl?uri=[% MARCurl.MARCURL | uri %]&biblionumber=[% biblio.biblionumber | uri %]" title="[% MARCurl.MARCURL | html %]" target="_blank" rel="noreferrer"> Yes, uri filter, because it is token-y. uri=<url thing>. @@ +341,2 @@ > [% ELSE %] > + <a href="[% MARCurl.MARCURL | url %]" title="[% MARCurl.MARCURL | html %]" target="_blank" rel="noreferrer"> Whereas, this is the URL to use. ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt @@ +71,4 @@ > </div> > <div id="rssnews-container"> > <!-- Logged in users have a branch code or it could be explicitly set --> > + <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-news-rss.pl?branchcode=[% branchcode | uri %]"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png"></a> The img src url.... is interface url? is theme just a piece of the url? Something feels wrong about this, but I could be wrong. ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt @@ +20,4 @@ > <channel> > <title><![CDATA[[% LibraryName | html %] Search [% IF ( query_desc ) %]for '[% query_desc | html %]'[% END %][% IF ( limit_desc ) %] with limit(s): '[% limit_desc | html %]'[% END %]]]></title> > <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% query_cgi | html %][% limit_cgi | html %]&format=rss2</link> > + <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |uri %][% limit_cgi |uri %]&sort_by=[% sort_by | uri %]&format=rss2"/> It's weird that the query_cgi and limit_cgi lack an & here. Something feels weird, but no sense busting it. ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt @@ +18,4 @@ > [% END %] > [% END %] > > +<link rel="alternate" type="application/rss+xml" title="[% LibraryName | html %] Search RSS feed" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |url %]&count=[% countrss |uri %]&sort_by=acqdate_dsc&format=rss2" /> opac-opensearch.tt had uri and uri on the query and limit. But like I said, this lack of & feels weird. @@ +43,4 @@ > <strong>No results found!</strong> > <p> > [% IF ( searchdesc ) %] > + No results found for that in [% LibraryName | html %] catalog. <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi | url %]&format=rss2" class="rsssearchlink noprint"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" border="0" class="rsssearchicon"/></a> But like I said before, I'm not sure url is the correct filter. There's some lack of consistency on how to treat them. @@ -43,4 @@ > <strong>No results found!</strong> > <p> > [% IF ( searchdesc ) %] > - No results found for that in [% LibraryName | html %] catalog. <a href="[% OPACBaseURL | uri %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi | html | url %]&format=rss2" class="rsssearchlink noprint"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" border="0" class="rsssearchicon"/></a> | html | url?! What was that?! That was clearly wrong. :) @@ +87,4 @@ > [% END %] > ). > [% END %] > + <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi | url %]&count=[% countrss | uri %]&sort_by=acqdate_dsc&format=rss2" class="rsssearchlink noprint"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> On a positive note, it should be OPACBaseURL | url. :) ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-search-history.tt @@ +71,4 @@ > <tr> > <td><input type="checkbox" name="id" value="[% s.id | html %]" /></td> > <td><span title="[% s.time | html %]">[% s.time |$KohaDates with_hours => 1 | html %]</span></td> > + <td><a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |url %][% limit_cgi | url %]&[% s.query_cgi | uri %]&count=[% countrss | uri %]&sort_by=acqdate_dsc&format=rss2" class="rsssearchlink"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> <a href="/cgi-bin/koha/opac-search.pl?[% s.query_cgi | html %]">[% s.query_desc | html %]</a></td> So why is s.queru_cgi merit a uri, but query_cgi a url? What is the weirdness with query_cgi? Does it have an &? -- Yuck. The s.query_cgi near the end is still through | html even though it is part of an href. @@ +111,4 @@ > <tr> > <td><input type="checkbox" name="id" value="[% s.id | html %]" /></td> > <td><span title="[% s.time | html %]">[% s.time |$KohaDates with_hours => 1 | html %]</span></td> > + <td><a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |url %][% limit_cgi | url %]&[% s.query_cgi | uri %]&count=[% countrss | uri %]&sort_by=acqdate_dsc&format=rss2" class="rsssearchlink"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> <a href="/cgi-bin/koha/opac-search.pl?[% s.query_cgi | html %]">[% s.query_desc | html %]</a></td> Same yuckiness here. ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews.tt @@ +29,4 @@ > <div class="span12"> > [% END %] > <div id="showreviews" class="searchresults maincontent"> > + <h3>Recent comments <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-showreviews.pl?format=rss" class="rsssearchlink"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to recent comments" title="Subscribe to recent comments" border="0" class="rsssearchicon"/></a></h3> I know I've talked about interface before, but can someone clarify what values I might expect, so I can think about whether that filter is right. Same with theme. ::: misc/cronjobs/rss/lastAcquired-1.0.tt @@ +32,4 @@ > <br>[% place | html %] [% i.publishercode | html %] [% i.publicationyear | html %] > <br>[% pages | html %] [% i.illus | html %] [% i.size | html %] > [% IF i.notes %]<br><br>[% i.notes | html %][% END %]<br> > +<a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-detail.pl?biblionumber=[% i.biblionumber | uri %]">View Details</a> | <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-reserve.pl?biblionumber=[% i.biblionumber | uri %]">Reserve this Item</a>[% IF i.author %] | <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?marclist=biblio.author&and_or=and&excluding=&operator=contains&value=[% i.author | uri %]&resultsperpage=20&orderby=biblio.title&op=do_search">More by this Author</a>[% END %] Single line monstrosities are really hard to eyeball, perhaps a follow up patch to better indent the modified sections across multiple lines?
Created attachment 81763 [details] [review] Bug 21526: (follow-up) Use the 'url' filter when needed
Created attachment 81764 [details] [review] Bug 21526: Fix search result pages (url vs uri vs raw) query_cgi is uri_escaped from the pl, so we should displayed as raw Test plan: Use wide characters ❤ Search, filter, facets, search history, rss (both interfaces) Please test deeply!
(In reply to M. Tompsett from comment #21) > Comment on attachment 81530 [details] [review] [review] > Bug 21526: Use the 'url' filter when needed > > Review of attachment 81530 [details] [review] [review]: Thanks Mark! > > + [% link = BLOCK %]<a href="[% authidurl _ authid | url %]">[% heading | html %]</a>[% END %] > > Perhaps this line and line 31 should use the same method to build the URL? I > tend to prefer 31's split. I do not understand what you mean. > > + <fieldset class="action"><input type="hidden" name="op" value="[% op | html %]" />[% IF ( suggestionid ) %]<input type="submit" value="Save" /> <a class="cancel" href="[% IF ( returnsuggestedby ) %]/cgi-bin/koha/members/moremember.pl?borrowernumber=[% returnsuggestedby | uri %]#suggestions[% ELSE %]suggestion.pl[% END %]">Cancel</a>[% ELSE %]<input type="submit" value="Submit your suggestion" /> <a class="cancel" href="suggestion.pl">Cancel</a>[% END %] > > Nice correction of html to uri in the url build part, but the hidden value's > value... perhaps uri? After all, it's like the portion of a long GET > request, not a string to display. What are you suggestion here? > > + <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-news-rss.pl?branchcode=[% branchcode | uri %]"><img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png"></a> > > The img src url.... is interface url? is theme just a piece of the url? > Something feels wrong about this, but I could be wrong. "interface" must be "intranet-tmpl" or "opac-tmpl" "theme" must be "bootstrap" or "prog" => I did not take care of them, not important for now. I have fixed the search/facets on a separate bug reports. Other comments have been fixed or ignored when I disagree. Please provide tests to recreate an error if you think you find some. Reopening the bug, as I think it must be fixed here. The last 3 patches need to be tested.
Upping severity to get the attention it needs.
Created attachment 82011 [details] [review] Bug 21526: Use the 'url' filter when needed Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 82012 [details] [review] Bug 21526: (follow-up) Use the 'url' filter when needed Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 82013 [details] [review] Bug 21526: Fix search result pages (url vs uri vs raw) query_cgi is uri_escaped from the pl, so we should displayed as raw Test plan: Use wide characters ❤ Search, filter, facets, search history, rss (both interfaces) Please test deeply! Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
QA: Looking here
(In reply to M. Tompsett from comment #21) > Nice catch on the URL vs URI. But I think URI should be used for the base > part, and URL for anything we are adding to it, unless there is a bizarre & > at the end or something. AFAIK uri should be used to encode parameters. And does more than url which should be used for the URL :) So especially for the path component. Not sure what you exactly mean by "base part" here.. But somehow the above statement does not feel completely right..
koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc [% url = BLOCK %][% url | $raw %][% "&sort_by=" _ sort_by | url %][% END %] So sort_by should be filtered by uri. [% local_url = BLOCK %][% url | $raw %][% "&nolimit=" _ facet.type_link_value _ ":" _ facet.facet_link_value | url %][% END %] Same here facet_link_value should be filtered by uri. [% local_url = BLOCK %][% url | $raw %][% "&limit=" _ facet.type_link_value _ ":" _ facet.facet_link_value | url %][% END %] Same line again Since you normally filter params with uri, you have a reason here. Could you document such exceptions (or correct them if no reason) ?
IF ( subfiel.is_url ) Nothing for you but how ugly. Existing choice of variables..
koha-tmpl/intranet-tmpl/prog/en/modules/circ/printslip.tt +<link rel="shortcut icon" href="[% IF ( IntranetFavicon ) %][% IntranetFavicon | url %][% ELSE %][% interface | html %]/[% theme | html %]/img/favicon.ico[% END %]" type="image/x-icon" /> interface and theme are parts here of the url path and not part of a html fragment so they should go here through url (not html or uri)
<a href="[% cr.item.uri | url %]">Item URI</a> Nice but perfect :)
koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc [% url = BLOCK %][% url | $raw %][% "&sort_by=" _ sort_by | url %][% END %] Same as for facets above
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt <img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png"> interface and theme here are path parts so should go thru url
+ <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | url %]&format=rss2</link> + <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | url %]&sort_by=[% sort_by | uri %]&format=rss2"/> limit_cgi is a parameter, uri needed ?
(In reply to Marcel de Rooy from comment #37) > + <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% > query_cgi | $raw %][% limit_cgi | url %]&format=rss2</link> > + <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL > | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | > url %]&sort_by=[% sort_by | uri %]&format=rss2"/> > > limit_cgi is a parameter, uri needed ? koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt And more occurrences. Seems deliberate, not documented.
<img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/ As above. url instead of html in src attribute
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-search-history.tt <a href="[% OPACBaseURL | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi |url %][% limit_cgi | url %]&[% s.query_cgi | uri %]& Funny mixup. Your script picks url for query_cgi again (for some reason) but does not fir s.query_cgi.
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews.tt <img src="[% interface | html %]/[% theme | html %]/images/feed-icon-16x16.png" Same as above
koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/help.tt [% interface | html %]/[% theme | html %] Same again koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/printslip.tt [% interface | html %]/[% theme | html %] Same again koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt [% interface | html %]/[% theme | html %]
Tested little bit with the facets. Included & in author name. Searching for Foy & Foy (from learning perl :) /cgi-bin/koha/opac-search.pl?idx=kw&q=a&sort_by=relevance_dsc&limit=au:Foy%20&%20Foy This is the result of url and it works better than the uri version. But actually both are not working so good. /cgi-bin/koha/opac-search.pl?idx=kw&q=a&sort_by=relevance_dsc%26limit%3Dau%3AFoy%20%26amp%3B%20Foy This comes from uri. Does not work. Conclusion: This problem is greater than just a filter. Keep url as best of both. Needs attention somewhere else. Probably the unparalleled C4::Search.
Created attachment 82140 [details] [review] Bug 21526: Use the 'url' filter when needed Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 82141 [details] [review] Bug 21526: (follow-up) Use the 'url' filter when needed Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 82142 [details] [review] Bug 21526: Fix search result pages (url vs uri vs raw) query_cgi is uri_escaped from the pl, so we should displayed as raw Test plan: Use wide characters ❤ Search, filter, facets, search history, rss (both interfaces) Please test deeply! Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I recommend to push these patches now. Some refinements can follow (see above comments). And Search needs attention, which we knew already..
(In reply to Marcel de Rooy from comment #31) > koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc > [% url = BLOCK %][% url | $raw %][% "&sort_by=" _ sort_by | url %][% END %] > So sort_by should be filtered by uri. Yes. > [% local_url = BLOCK %][% url | $raw %][% "&nolimit=" _ > facet.type_link_value _ ":" _ facet.facet_link_value | url %][% END %] > Same here facet_link_value should be filtered by uri. Yes but more complex then, we should not only replace the filter. > [% local_url = BLOCK %][% url | $raw %][% "&limit=" _ facet.type_link_value > _ ":" _ facet.facet_link_value | url %][% END %] > Same line again > > Since you normally filter params with uri, you have a reason here. Could you > document such exceptions (or correct them if no reason) ? uri will replace the &, we do not want that (see bellow) (In reply to Marcel de Rooy from comment #33) > koha-tmpl/intranet-tmpl/prog/en/modules/circ/printslip.tt > +<link rel="shortcut icon" href="[% IF ( IntranetFavicon ) %][% > IntranetFavicon | url %][% ELSE %][% interface | html %]/[% theme | html > %]/img/favicon.ico[% END %]" type="image/x-icon" /> > interface and theme are parts here of the url path and not part of a html > fragment so they should go here through url (not html or uri) Yes, see comment 24, I ignored them. (In reply to Marcel de Rooy from comment #35) > koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc > [% url = BLOCK %][% url | $raw %][% "&sort_by=" _ sort_by | url %][% END > %] > Same as for facets above (In reply to Marcel de Rooy from comment #37) > + <link>[% OPACBaseURL | html %]/cgi-bin/koha/opac-search.pl?[% > query_cgi | $raw %][% limit_cgi | url %]&format=rss2</link> > + <atom:link rel="self" type="application/rss+xml" href="[% OPACBaseURL > | url %]/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | > url %]&sort_by=[% sort_by | uri %]&format=rss2"/> > > limit_cgi is a parameter, uri needed ? (In reply to Marcel de Rooy from comment #47) > I recommend to push these patches now. Some refinements can follow (see > above comments). And Search needs attention, which we knew already.. The search part is tricky, and we should rewrite it. Did you find regressions since 18.05? If you did we really need to take care of them (it's critical).
Pushed for 18.11. Awesome work all!
Bug 13618 not in 18.05.x series, no requirement to backport.
Created attachment 82600 [details] [review] Bug 21526 - To fix pagination problem in OPAC Due to conversions from html to uri in various places pagination in OPAC doesnot work properly. This change will resolve pagination problem.
Comment on attachment 82600 [details] [review] Bug 21526 - To fix pagination problem in OPAC Has been moved to 21878
Please submit further fixes to NEW reports.
requires 21526 wont backport to 18.05
*** Bug 13812 has been marked as a duplicate of this bug. ***