https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21526
--- Comment #21 from M. Tompsett <mtomp...@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. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
