https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21526 --- Comment #21 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 81530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81530 Bug 21526: Use the 'url' filter when needed Review of attachment 81530: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=21526&attachment=81530) ----------------------------------------------------------------- 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? -- You are receiving this mail because: You are watching all bug changes.